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

windows: use built-in _WIN32 macro to detect Windows #12376

Closed
wants to merge 8 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Nov 21, 2023

Windows compilers define _WIN32 automatically. Windows SDK headers
or build env defines WIN32, or we have to take care of it. The
agreement seems to be that _WIN32 is the preferred practice here.
Make the source code rely on that to detect we're building for Windows.

Public curl.h was using WIN32, __WIN32__ and CURL_WIN32 for
Windows detection, next to the official _WIN32. After this patch it
only uses _WIN32 for this. Also, make it stop defining CURL_WIN32.

There is a slight chance these break compatibility with Windows
compilers that fail to define _WIN32. I'm not aware of any obsolete
or modern compiler affected, but in case there is one, one possible
solution is to define this macro manually.

grepping for WIN32 remains useful to discover Windows-specific code.

Also:

  • extend checksrc to ensure we're not using WIN32 anymore.

  • apply minor formatting here and there.

  • delete unnecessary checks for !MSDOS when _WIN32 is present.

Closes #12376

- Warn via checksrc if symbol WIN32 is used in preprocessor lines.
@jay
Copy link
Member

jay commented Nov 21, 2023

My initial reaction to this was it's unnecessary, even though it is "more correct". We already use WIN32 and it works fine. However it is, yes, more correct. My biggest concern I think is that we will occasionally forget WIN32 is now _WIN32 and make the mistake of using the former. To that end I've added a checksrc warning category BANNEDPREPROC that checks for banned symbols (eg WIN32) on preprocessor lines.

- Suppress checksrc warning WIN32 macro eval for CURL_WIN32
@vszakats
Copy link
Member Author

Good idea to make checksrc check for this.

I'm thinking to sync curl/curl.h with this and replace
CURL_WIN32 with _WIN32, to avoid the minor chance of
it disagreeing with what 'win32' is in the internals and causing
issues.

CURL_WIN32 seems to be a documented feature of curl.h
since 7.69.0, so we might want to keep it around for compatibility,
if we're convinced this is useful or necessary. I tend to think it isn't.

This would be a reboot of 1adebe7
and 8bd863f + #4854 + #4855, but
instead of keeping around CURL_WIN32 as a replacement for our local
WIN32 we just rely on _WIN32, which works universally out of the box.

What do you think?

@jay
Copy link
Member

jay commented Nov 21, 2023

I struggle to think of when _WIN32 isn't available. Let's see if anyone else has anything to say on it...

Ref: #4855

@vszakats
Copy link
Member Author

vszakats commented Nov 22, 2023

Some references:

One list mentions Borland C++, so I made a test with
its 5.5.1 version from 2000, and it does define _WIN32.
I also tried Open Watcom 1.9 from 2010 and it does too.
Pelles C most definitely had it because it was emulating
MSVC in most aspects.

Can't think of more niche compilers that are still around.

But if there are, they can almost certainly set -D_WIN32
manually at build time. Not just for curl.

#if (defined(_WIN32) || defined(__WIN32__) || defined(WIN32)) && \
#if (defined(_WIN32) || \
defined(WIN32) /* !checksrc! disable BANNEDPREPROC 1 */ || \
defined(__WIN32__)) && \
!defined(__SYMBIAN32__)
Copy link
Member Author

@vszakats vszakats Nov 22, 2023

Choose a reason for hiding this comment

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

It's curious that _WIN32 or __WIN32__ could end up together with __SYMBIAN32__,
so that it had to be guarded-out.

Added in 1960eeb (2008). Anyhow, Symbian was deprecated
in curl in 3d64031 (2020), so this no longer a concern.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ref: #12378

Copy link
Contributor

Choose a reason for hiding this comment

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

It's curious that _WIN32 or __WIN32__ could end up together with __SYMBIAN32__, so that it had to be guarded-out.

It's been too long for me to recall for certain, but it may have been for x86 builds targeting the Symbian emulator running on Win32.

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

I'm fine with also removing CURL_WIN32 from the public header. We never documented it for any purpose. It is mentioned in symbols-in-version simply because it is present in curl.h but I don't think we break any promises (API/ABI) by taking it away again.

This is a reboot of 1adebe7 and 8bd863f + curl#4854 + curl#4855, but instead of
keeping around CURL_WIN32 as a replacement for our local WIN32 we just
rely on _WIN32, which works universally out of the box.

Ref: curl#12376 (comment)
```
 Missing symbols mentioned in symbols-in-versions
 Add them to a header, or mark them as removed.
 CURL_WIN32
```
Ref: https://github.com/curl/curl/actions/runs/6956411932/job/18927212026#step:11:1957
@vszakats vszakats closed this in e9a7d4a Nov 22, 2023
@vszakats vszakats deleted the use-_WIN32 branch November 22, 2023 15:45
vszakats added a commit to vszakats/curl that referenced this pull request Jun 4, 2024
This fix was supposed to be committed earlier, but ended up missing from
the final commit.

Follow-up to e9a7d4a curl#12376
Closes #xxxxx
vszakats added a commit that referenced this pull request Jun 4, 2024
This fix was supposed to be committed earlier, but ended up missing from
the final commit.

Follow-up to e9a7d4a #12376
Closes #13878
vszakats added a commit to vszakats/curl that referenced this pull request Sep 19, 2024
vszakats added a commit that referenced this pull request Sep 19, 2024
moritzbuhl pushed a commit to moritzbuhl/curl that referenced this pull request Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants