Fix wrong config defines in boost/windows.cfg#6492
Fix wrong config defines in boost/windows.cfg#6492firewave merged 4 commits intocppcheck-opensource:mainfrom
Conversation
|
Thanks! Please add some test cases at test/cfg as well |
|
Please rebase, there was a CI failure. |
| <define name="INVALID_HANDLE_VALUE" value="0"/> | ||
| <define name="INVALID_SOCKET" value="0"/> | ||
| <define name="INVALID_HANDLE_VALUE" value="-1"/> | ||
| <define name="INVALID_SOCKET" value="~0"/> |
There was a problem hiding this comment.
should INVALID_SOCKET also be -1 ? ~0 is -1 if 2-complement is used.
There was a problem hiding this comment.
-1 would do the same thing as ~0. I decided to use ~0 as INVALID_SOCKET is defined like this in winsock.h, probably because SOCKET is unsigned. Should I rather use -1?
There was a problem hiding this comment.
Is there actual documentation of the value of INVALID_SOCKET? IIRC depending on the license we cannot simply copy contents from existing headers as that would be a violation - MinGW (or MSYS?) got into hot water about this a long time ago - I cannot find anything about it though.
https://learn.microsoft.com/en-us/windows/win32/winsock/socket-data-type-2 seems to imply it is -1 or negative.
There was a problem hiding this comment.
It might have actually been Cygwin and might be related to this: https://www.cygwin.com/faq.html#faq.programming.win32-headers.
There was a problem hiding this comment.
We could use the headers of MinGW as source but that licensing seems to be quite convoluted with a mixture of various ones depending on the file.
There was a problem hiding this comment.
Even Microsoft cannot copyright the value of a constant, and we are not copying/redistributing their headers or implementing their API anyway.
What's more concerning is that we define SOCKET as just int, whereas it should be __int64 or equivalent on 64bit platforms. But that's probably out of scope for this PR.
There was a problem hiding this comment.
Unfortunately I can't find any documentation on the value of INVALID_SOCKET and I can't find a mirror of the windows headers that I could refer to.
I came up with the ~0 because I looked this up in the WinSock2.h on my Windows machine. I did not copy paste anything as this would introduce yet another warning because of the C-style cast used there to cast the ~0 into a SOCKET. The SOCKET itself is defined as a UINT_PTR and this is defined in BaseTsd.h as unsigned __int64 for 64 bit or as unsigned int for 32 bit.
Here:
https://learn.microsoft.com/en-us/windows/win32/winsock/handling-winsock-errors
they mention a value of 0xffff which is probably not correct on a 64 bit machine.
Here I found a winsock.h in the Microsoft repo on GitHub:
https://github.com/microsoft/service-fabric/blob/bc5c2bd546e98812509330273ae1bc6502276f87/src/prod/src/pal/src/winsock2.h#L56
there the INVALID_SOCKET is the same as on my machine but there SOCKET is only an int.
There was a problem hiding this comment.
I am not sure how to handle this. I personally only added stuff which was explicitly documented but I cannot speak for other submissions. I just jumped on this because you explicitly mentioned the original header as a source.
Maybe this is just an issue of re-licensing so maybe the configuration files simply need to be put into the public domain instead of GPL. Or it is even a complete non-issue as we are not using it in code.
CC @danmar
There was a problem hiding this comment.
@firewave thanks for the hint, I have to admit that I hadn't paid enough attention to the license but I don't think we have a licensing issue here. I did not copy any code from any Microsoft header into the code that is redistributed here. I only used the original headers form the sdk as a source of information for the values and for my commit message.
What I used can be found under the MIT license or the BSD license in the Microsoft GitHub repos as well:
https://github.com/microsoft/service-fabric/blob/bc5c2bd546e98812509330273ae1bc6502276f87/src/prod/src/pal/src/winsock2.h#L56
https://github.com/microsoft/mu_plus/blob/2d6746b8fe04a9be3998b3e1a682a4cbffcd7345/XmlSupportPkg/Library/XmlTreeLib/fasterxml/fasterxml.h#L949
So I'm pretty sure that this was fine. However, I have now removed the explicit mention from the commit message and form the comments above.
There was a problem hiding this comment.
Great. Thanks for looking into this.
As this was quite a while ago (probably 15+ years - maybe even longer) it is likely the license has changed. Also it is no longer the Microsoft of old (at least in relation to Linux/open-source).
The _MESSAGE macros were causing an unusedScopedObject warning because the created string was not used. Casting the message to void fixed the warnings. Add missing `BOOST_AUTO_TEST_CASE_TEMPLATE` macro accidentally removed with d1b3670
INVALID_HANDLE_VALUE is defined as `-1` in the windows sdk INVALID_SOCKET is defined as `~0` in the windwos sdk Add missing generic rights
Add one test for the `BOOST_<level>_MESSAGE` macros Add one test for the `BOOST_AUTO_TEST_CASE_TEMPLATE`
There should be no Cppcheck warning when checking for 0 and `INVALID_HANDLE_VALUE` or `INVALID_SOCKET`
There was a problem hiding this comment.
lgtm
@firewave @chrchr-github do you feel we can merge this?
Fix defines for boost test macros
The _MESSAGE macros were causing an unusedScopedObject warning because
the created string was not used. Casting the message to void fixed the
warnings.
Add missing
BOOST_AUTO_TEST_CASE_TEMPLATEmacro accidentally removedwith d1b3670
Fix wrong defined in windows.cfg
INVALID_HANDLE_VALUE is defined as
-1in the windows sdkINVALID_SOCKET is defined as
~0in the windwos sdkAdd missing generic rights