Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests/server/util.h: align WIN32 condition with util.c #8594

Closed
wants to merge 2 commits into from

Conversation

mback2k
Copy link
Member

@mback2k mback2k commented Mar 14, 2022

  • tests/server/util.h: align WIN32 condition with util.c

    There is no need to test for both _WIN32 and WIN32 as curl_setup.h
    automatically defines the later if the first one is defined.

    Also tests/server/util.c is only checking for WIN32 arouund the
    implementation of win32_perror, so just defining _WIN32
    would not be sufficient for a successful compilation.

  • lib/warnless.[ch]: only check for WIN32 and ignore _WIN32

    curl_setup.h automatically defines WIN32 if just _WIN32 is defined.

@mback2k mback2k self-assigned this Mar 14, 2022
@jay
Copy link
Member

@jay jay commented Mar 15, 2022

This looks fine except for warnless.h which does not start with a #include "curl_setup.h" (therefore WIN32 possibly not yet defined). If I had to guess we omitted it because warnless.h is included by curlx.h. However, other header files that are included by curlx.h use #include "curl_setup.h" so I'm not sure what to make of it...

@mback2k
Copy link
Member Author

@mback2k mback2k commented Mar 15, 2022

Thanks @jay, I thought the same. Completely green CI seems to prove that the changed warnless.[ch] still works, but if we are not sure about the original intent, I may also drop the 2nd commit on merging this PR. @bagder do you maybe have insights?

@mback2k mback2k marked this pull request as ready for review Mar 15, 2022
@mback2k mback2k requested review from bagder and jay Mar 15, 2022
@jay
Copy link
Member

@jay jay commented Mar 15, 2022

I think at this point nobody outside the project is using curlx separately or curl_setup.h includes fine for them? I don't understand how that could be though. I think it's fine to add #include "curl_setup.h" to the top of warnless.h because the other curlx headers already include it and it's been like that for quite a while, years.

bagder
bagder approved these changes Mar 15, 2022
@bagder
Copy link
Member

@bagder bagder commented Mar 15, 2022

I would presume that all current users of warnless.h right now includecurl_setup.h themselves which makes this problem not appear.

@mback2k
Copy link
Member Author

@mback2k mback2k commented Mar 15, 2022

Thanks, @bagder is this a yes or no regarding @jay suggestions? In case of a no, I would merge the PR as it is now.

I think it's fine to add #include "curl_setup.h" to the top of warnless.h because the other curlx headers already include it and it's been like that for quite a while, years.

@mback2k mback2k requested a review from bagder Mar 20, 2022
mback2k added 2 commits Mar 20, 2022
There is no need to test for both _WIN32 and WIN32 as curl_setup.h
automatically defines the later if the first one is defined.

Also tests/server/util.c is only checking for WIN32 arouund the
implementation of win32_perror, so just defining _WIN32
would not be sufficient for a successful compilation.

Reviewed-by: Daniel Stenberg
Reviewed-by: Jay Satiro

Closes curl#8594
curl_setup.h automatically defines WIN32 if just _WIN32 is defined.

Therefore make sure curl_setup.h is included through warnless.h.

Reviewed-by: Daniel Stenberg
Reviewed-by: Jay Satiro

Closes curl#8594
@mback2k
Copy link
Member Author

@mback2k mback2k commented Mar 21, 2022

I added #include "curl_setup.h" to warnless.h. Do you guys think this is ready to be merged now? @bagder @jay

jay
jay approved these changes Mar 22, 2022
mback2k added a commit that referenced this issue Mar 23, 2022
curl_setup.h automatically defines WIN32 if just _WIN32 is defined.

Therefore make sure curl_setup.h is included through warnless.h.

Reviewed-by: Daniel Stenberg
Reviewed-by: Jay Satiro

Closes #8594
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants