lib: consolidate Win32 setup definitions in setup-win32.h #9375
Conversation
Suggested-by: Marcel Raad Follow up to curl#9312
lib/curl_setup.h
Outdated
| # endif | ||
| # ifndef NOGDI | ||
| # define NOGDI | ||
| # endif |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
According to this comment, it should be safe:
Line 210 in c3b4090
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.
There was a problem hiding this comment.
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
_UNICODEdefine makes no sense
I assume you are referring to this in setup-win32, which comes after curl.h is included (which includes windows.h):
Lines 38 to 46 in 45ac4d0
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>
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
@MarcelRaad @jay I am a little bit lost now, can you take it from here for me?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have no objections and @MarcelRaad's suggestion to try including system.h seems reasonable.
There was a problem hiding this comment.
Should I merge this PR as it is right now or will there be further changes? @jay @MarcelRaad
There was a problem hiding this comment.
I think this is good as it is! We can work on the curl.h include when this is merged.
- 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
- 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
The platform setup headers might set definitions required for the includes in curl.h. Ref: curl#9375 (comment) Closes
As done before commit 9506d01. Ref: curl#9375 (comment) Closes
As done before commit 9506d01. Ref: curl#9375 (comment) Closes
As done before commit 9506d01. Ref: curl#9375 (comment) Closes
The platform setup headers might set definitions required for the includes in curl.h. Ref: curl#9375 (comment) Closes curl#9453
As done before commit 9506d01. Ref: curl#9375 (comment) Closes curl#9453
- 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/curl#9375 (comment) Reported-by: Marcel Raad Closes curl/curl#9384
The platform setup headers might set definitions required for the includes in curl.h. Ref: curl/curl#9375 (comment) Closes curl/curl#9453
As done before commit 9506d01. Ref: curl/curl#9375 (comment) Closes curl/curl#9453
Suggested-by: Marcel Raad
Follow up to #9312