[GitHub] [logging-log4cxx] rm5248 edited a comment on pull request #42: fix build without wchar

[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