You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2020/09/24 17:14:42 UTC
[GitHub] [logging-log4cxx] ffontaine opened a new pull request #42: fix build without wchar
ffontaine opened a new pull request #42:
URL: https://github.com/apache/logging-log4cxx/pull/42
Disable wencode without wchar or the build will fail on:
```
odbcappender.cpp: In static member function 'static void log4cxx::db::ODBCAppender::encode(wchar_t**, const LogString&, log4cxx::helpers::Pool&)':
odbcappender.cpp:362:22: error: 'wencode' is not a member of 'log4cxx::helpers::Transcoder'
*dest = Transcoder::wencode(src, p);
^~~~~~~
```
Fixes:
- http://autobuild.buildroot.org/results/bab5329fdeb894471bfd5192ce04d3fbd2f9be5c
Signed-off-by: Fabrice Fontaine <fo...@gmail.com>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ffontaine commented on pull request #42: fix build without wchar
Posted by GitBox <gi...@apache.org>.
ffontaine commented on pull request #42:
URL: https://github.com/apache/logging-log4cxx/pull/42#issuecomment-698737347
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ffontaine commented on pull request #42: fix build without wchar
Posted by GitBox <gi...@apache.org>.
ffontaine commented on pull request #42:
URL: https://github.com/apache/logging-log4cxx/pull/42#issuecomment-698737347
I updated the PR to keep the `ODBCAppender::encode` function which won't do anything if `Transcoder::wencode` is not available.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 merged pull request #42: fix build without wchar
Posted by GitBox <gi...@apache.org>.
rm5248 merged pull request #42:
URL: https://github.com/apache/logging-log4cxx/pull/42
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on pull request #42: fix build without wchar
Posted by GitBox <gi...@apache.org>.
rm5248 commented on pull request #42:
URL: https://github.com/apache/logging-log4cxx/pull/42#issuecomment-698666494
Shouldn't this also #ifdef out the function signature in the .h file? Or would it make more sense to have the method implementation be just a stub? I'm thinking that you could still create something that used the encode method, which would then fail at link-time instead of compile-time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on pull request #42: fix build without wchar
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on pull request #42:
URL: https://github.com/apache/logging-log4cxx/pull/42#issuecomment-698767348
How does keeping `encode` as a no-op make sense here? Either it's called, then making it a no-op breaks on runtime only or it's not, then itself should be `#ifdef`ed in my opinion, not its implementation only.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on pull request #42: fix build without wchar
Posted by GitBox <gi...@apache.org>.
rm5248 commented on pull request #42:
URL: https://github.com/apache/logging-log4cxx/pull/42#issuecomment-699201041
> How does keeping `encode` as a no-op make sense here? Either it's called, then making it a no-op breaks on runtime only or it's not, then itself should be `#ifdef`ed in my opinion, not its implementation only.
There's two ways to do this:
1.
```
// header file
#ifdef SOME_VARIABLE
void foo();
#endf
// cpp file
#ifdef SOME_VARIABLE
void foo(){
// Some Implementation
bar();
}
#endif
```
2.
```
// header file
void foo();
// cpp file
#ifdef SOME_VARIABLE
void foo(){
// Some Implementation
bar();
}
#else
void foo(){
// No-op implemetation
}
#endif
```
The advantage of the first method is that if you don't have some feature, you can't use it in the code. The advantage of the second example is that the function always exists, so that you can always call it from any code without having the calling code to check if it exists. The second example is preferred for [Linux kernel coding](https://www.kernel.org/doc/html/v4.10/process/coding-style.html#conditional-compilation) for this reason. This does imply that you need to structure your code accordingly to make sure that there are no side-effects that you are expecting from the method you are calling, if it doesn't exist.
We have a (potential) problem with issues like this if you use objects such as the NTEventLogAppender. I can include the file in Linux, but as soon as I try to compile an application that uses it I get a linker error, as it is Windows-only. So if you are somehow using the NTEventLogAppender directly in a cross-platform application, you need to #ifdef out portions of your code instead of those operations being no-ops on Linux.
In this particular case, it seems like the first option is the best, since log4cxx may or may not have the `wchar_t` API.
-----
Otherwise, looks good to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 edited a comment on pull request #42: fix build without wchar
Posted by GitBox <gi...@apache.org>.
rm5248 edited a comment on pull request #42:
URL: https://github.com/apache/logging-log4cxx/pull/42#issuecomment-699201041
> How does keeping `encode` as a no-op make sense here? Either it's called, then making it a no-op breaks on runtime only or it's not, then itself should be `#ifdef`ed in my opinion, not its implementation only.
There's two ways to do this:
1.
```
// header file
#ifdef SOME_VARIABLE
void foo();
#endf
// cpp file
#ifdef SOME_VARIABLE
void foo(){
// Some Implementation
bar();
}
#endif
```
2.
```
// header file
void foo();
// cpp file
#ifdef SOME_VARIABLE
void foo(){
// Some Implementation
bar();
}
#else
void foo(){
// No-op implemetation
}
#endif
```
The advantage of the first method is that if you don't have some feature, you can't use it in the code. The advantage of the second example is that the function always exists, so that you can always call it from any code without having the calling code to check if it exists. The second example is preferred for [Linux kernel coding](https://www.kernel.org/doc/html/v4.10/process/coding-style.html#conditional-compilation) for this reason. This does imply that you need to structure your code accordingly to make sure that there are no side-effects that you are expecting from the method you are calling, if it doesn't exist.
We have a (potential) problem with issues like this if you use objects such as the NTEventLogAppender. I can include the file in Linux, but as soon as I try to compile an application that uses it I get a linker error, as it is Windows-only. So if you are somehow using the NTEventLogAppender directly in a cross-platform application, you need to #ifdef out portions of your code instead of those operations being no-ops on Linux.
In this particular case, it seems like the first option is the best, since log4cxx may or may not have the `wchar_t` API.
-----
Otherwise, looks good to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on pull request #42: fix build without wchar
Posted by GitBox <gi...@apache.org>.
rm5248 commented on pull request #42:
URL: https://github.com/apache/logging-log4cxx/pull/42#issuecomment-698666494
Shouldn't this also #ifdef out the function signature in the .h file? Or would it make more sense to have the method implementation be just a stub? I'm thinking that you could still create something that used the encode method, which would then fail at link-time instead of compile-time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ams-tschoening commented on pull request #42: fix build without wchar
Posted by GitBox <gi...@apache.org>.
ams-tschoening commented on pull request #42:
URL: https://github.com/apache/logging-log4cxx/pull/42#issuecomment-698767348
How does keeping `encode` as a no-op make sense here? Either it's called, then making it a no-op breaks on runtime only or it's not, then itself should be `#ifdef`ed in my opinion, not its implementation only.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] ffontaine commented on pull request #42: fix build without wchar
Posted by GitBox <gi...@apache.org>.
ffontaine commented on pull request #42:
URL: https://github.com/apache/logging-log4cxx/pull/42#issuecomment-698769494
I updated the PR following the second suggestion of @rm5248 but perhaps I misunderstood it. I'll update the PR again to ifdef `ODBCAppender::encode`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org