astitcher commented on issue #156: PROTON-1936: lower case include and updated 
ifdefs
URL: https://github.com/apache/qpid-proton/pull/156#issuecomment-465778368
 
 
   >
   >     1. **File name casing**. The windows API is not really very consistent 
when is comes to the casing of file names (can differ per VS version, or 
platform), so as windows file systems are not case sensitive and linux file 
systems are, inconsistent casing causes a problem on linux (i.e. `fatal error: 
Windows.h: No such file or directory # include <BaseTsd.h>`). Mingw "overcomes" 
this by lower casing all include and library files. So one part of this pull 
request lower cases all includes and libraries. As windows is case insensitive, 
this changes nothing on windows but allows compilation to succeed on linux.
   
   I'm completely fine with these changes.
   
   >
   >     2. **Preprocessor based platform detection** In several places in the 
proton code base platform detection is being done using preprocessor 
definitions. In a few of these cases it is doing so by using a define that 
identifies the compiler, not the platform. In these places I have added a check 
for a define that identifies the _platform_ (which as you mention should be the 
correct way of doing so). I did not feel confident enough to remove the 
existing compiler identification check, but I would be happy to do so.
   
   A few points here:
   * An aside really, but I would say that if you are using Windows you'd be 
better to use the schannel TLS implementation rather than openssl. But having 
said that the test in openssl.c should definitely be a platform test not a 
compiler test.
   * However I'm not at all clear whether the tests around snprintf, vsnprintf, 
va_copy are actually platform dependent.
     * snprintf, vsnprintf are C runtime library things, so they could either 
come from the platform or the compiler. I had assumed they came from the 
compiler in which case they don't need any new selection logic, but if they 
come from the platform then the selection login should be changed. 
     * va_copy is definitely compiler dependent and not platform dependent 
although I guess it might depend on the ABI in use.
   
   Did you find that you needed both these changes to make the code compile or 
work?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org

Reply via email to