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

lib: consolidate Win32 setup definitions in setup-win32.h #9375

Closed
wants to merge 2 commits into from

Conversation

mback2k
Copy link
Member

@mback2k mback2k commented Aug 26, 2022

Suggested-by: Marcel Raad

Follow up to #9312

@mback2k mback2k added the Windows Windows-specific label Aug 26, 2022
@mback2k mback2k requested review from jay and MarcelRaad Aug 26, 2022
@mback2k mback2k self-assigned this Aug 26, 2022
Copy link
Member

@MarcelRaad MarcelRaad left a comment

Thanks!

lib/curl_setup.h Outdated
# endif
# ifndef NOGDI
# define NOGDI
# endif
Copy link
Member

@jay jay Aug 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is a good idea to remove it here, we want it defined as early as possible in case anything that comes after includes something that includes windows.h

Copy link
Member

@MarcelRaad MarcelRaad Aug 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this comment, it should be safe:

/* No system header file shall be included in this file before this */

But right, probably all these platform setup includes should be before the curl.h include. Any idea why they're not? Especially the _UNICODE define makes no sense after including any C standard library headers.

Copy link
Member

@jay jay Aug 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why curl.h is included before the system includes, that's a good question, it wasn't always like that. 9506d01 added it @bagder

Especially the _UNICODE define makes no sense

I assume you are referring to this in setup-win32, which comes after curl.h is included (which includes windows.h):

curl/lib/setup-win32.h

Lines 38 to 46 in 45ac4d0

#ifdef HAVE_WINDOWS_H
# if defined(UNICODE) && !defined(_UNICODE)
# define _UNICODE
# endif
# if defined(_UNICODE) && !defined(UNICODE)
# define UNICODE
# endif
# include <winerror.h>
# include <windows.h>

I think we didn't notice because all build systems that can make a Windows Unicode build define both UNICODE and _UNICODE together. It could be changed to a sanity check

diff --git a/lib/setup-win32.h b/lib/setup-win32.h
index c16928d..6e897bc 100644
--- a/lib/setup-win32.h
+++ b/lib/setup-win32.h
@@ -37,10 +37,10 @@
 
 #ifdef HAVE_WINDOWS_H
 #  if defined(UNICODE) && !defined(_UNICODE)
-#    define _UNICODE
+#    error "UNICODE is defined but _UNICODE is not defined"
 #  endif
 #  if defined(_UNICODE) && !defined(UNICODE)
-#    define UNICODE
+#    error "_UNICODE is defined but UNICODE is not defined"
 #  endif
 #  include <winerror.h>
 #  include <windows.h>

Copy link
Member

@bagder bagder Aug 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I recall it, my change in 9506d01 was primarily to stop including the other headers we don't have anymore, and it seemed to work so I didn't think twice about it...

Copy link
Member Author

@mback2k mback2k Aug 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarcelRaad @jay I am a little bit lost now, can you take it from here for me?

Copy link
Member

@jay jay Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a commit with my suggestion which is don't remove it from curl-setup. Separately from that I will propose a PR to change the UNICODE logic as mentioned earlier but commit it when the feature window opens. As to curl.h include I think it is probably? ok to move it to the end. If @bagder is ok with it we can try moving it to the end after the feature window opens. Include order is like jenga sometimes.

Copy link
Member

@MarcelRaad MarcelRaad Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me! Also, including system.h instead of curl.h should probably be sufficient now that curlbuild.h and curlrules.h are gone.

Copy link
Member

@bagder bagder Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no objections and @MarcelRaad's suggestion to try including system.h seems reasonable.

Copy link
Member Author

@mback2k mback2k Sep 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I merge this PR as it is right now or will there be further changes? @jay @MarcelRaad

Copy link
Member

@MarcelRaad MarcelRaad Sep 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good as it is! We can work on the curl.h include when this is merged.

jay added a commit to jay/curl that referenced this pull request Aug 29, 2022
- If UNICODE or _UNICODE is defined but the other isn't then error
  instead of implicitly defining it.

As Marcel pointed out it is too late at this point to make such a define
because Windows headers may already be included, so likely it never
worked. We never noticed because build systems that can make Windows
Unicode builds always define both. If one is defined but not the other
then something went wrong during the build configuration.

Bug: curl#9375 (comment)
Reported-by: Marcel Raad

Closes #xxxx
@mback2k mback2k closed this in 0c68e25 Sep 6, 2022
jay added a commit that referenced this pull request Sep 7, 2022
- If UNICODE or _UNICODE is defined but the other isn't then error
  instead of implicitly defining it.

As Marcel pointed out it is too late at this point to make such a define
because Windows headers may already be included, so likely it never
worked. We never noticed because build systems that can make Windows
Unicode builds always define both. If one is defined but not the other
then something went wrong during the build configuration.

Bug: #9375 (comment)
Reported-by: Marcel Raad

Closes #9384
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Sep 7, 2022
The platform setup headers might set definitions required for the
includes in curl.h.

Ref: curl#9375 (comment)
Closes
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Sep 7, 2022
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Sep 7, 2022
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Sep 7, 2022
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Sep 12, 2022
The platform setup headers might set definitions required for the
includes in curl.h.

Ref: curl#9375 (comment)
Closes curl#9453
MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Windows Windows-specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants