Skip to content

Conversation

@sipa
Copy link
Collaborator

@sipa sipa commented May 6, 2021

This includes various build system and compatibility improvements discovered in the process of subtreeing this into Bitcoin Core (see bitcoin/bitcoin#21859).

  • A few compatibility issues suggested here by sipsorcery.
  • Support for __builtin_clz equivalent on MSVC
  • Support for CPUID detection on MSVC
  • Prevent internal symbols from being exported
  • Make old MinGW CI work (-all-static instead of -static needed).

@sipa
Copy link
Collaborator Author

sipa commented May 6, 2021

cc @sipsorcery

@gmaxwell
Copy link
Contributor

gmaxwell commented May 6, 2021

I believe _WIN32 is the standard rather than WIN32.

@sipsorcery
Copy link

I believe _WIN32 is the standard rather than WIN32.

I used WIN32 as it was the first existing example I came across in the Bitcoin Core code base. Taking a closer look both WIN32 and _WIN32 are both in common use. That being said, and as @gmaxwell alludes to, _WIN32 is the preprocessor define preset by the msvc compiler.

For other projects I usually use _MSVC_VER to differentiate an msvc build from a gcc or other compiler build. Not that relevant here since the defines are being used to select headers rather than compiler features.

I personally have no objections to either using WIN32 or _WIN32. In either case the intent is clear.

@gmaxwell
Copy link
Contributor

gmaxwell commented May 6, 2021

I believe that _WIN32 is set by the compiler on windows, WIN32 is only set if you have some header that sets it or a build environment that does.

https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros?redirectedfrom=MSDN&view=msvc-160

Googling around I see commits in some open source projects changing from WIN32 to _WIN32, and I see that some projects check for either or. I don't see any changing from _WIN32 to WIN32. So I think checking if either is set or just checking _WIN32 alone is the most reliable choice. As far as bitcoin core goes, it has a relatively limited support of build environments, so whatever works is fine. It's better though if library code tends to be more portable.

@sipa sipa force-pushed the 202105_sipsorcery branch from 8663d4f to d2832f0 Compare May 7, 2021 03:20
@sipa
Copy link
Collaborator Author

sipa commented May 7, 2021

Changed to _WIN32, and also switched unistd.h for stddef.h (as I think it's just for size_t).

@sipa sipa force-pushed the 202105_sipsorcery branch 6 times, most recently from 4809cc9 to 4721111 Compare May 7, 2021 06:35
@gmaxwell
Copy link
Contributor

gmaxwell commented May 7, 2021

utACK 4721111 this should be tested by someone on MSVC.

@sipa
Copy link
Collaborator Author

sipa commented May 7, 2021

I'm testing this in bitcoin/bitcoin#21859, so still making changes.

@sipa sipa force-pushed the 202105_sipsorcery branch 9 times, most recently from 5095597 to c03320f Compare May 8, 2021 20:10
@sipa sipa changed the title A few fixes to prepare for MSVC building A few improvements to prepare for subtree in Bitcoin Core May 8, 2021
@sipa sipa force-pushed the 202105_sipsorcery branch 2 times, most recently from 371d170 to ace43f2 Compare May 8, 2021 21:06
@sipa sipa force-pushed the 202105_sipsorcery branch from ace43f2 to 27468da Compare May 11, 2021 21:01
@sipa sipa force-pushed the 202105_sipsorcery branch 2 times, most recently from 5d26cc6 to 20f1136 Compare May 11, 2021 21:39
@sipa sipa force-pushed the 202105_sipsorcery branch 12 times, most recently from d37e09f to ee197e0 Compare May 14, 2021 05:50
@sipa sipa force-pushed the 202105_sipsorcery branch from ee197e0 to 12d2f29 Compare May 15, 2021 22:23
@sipa sipa force-pushed the 202105_sipsorcery branch from 12d2f29 to 1e96b67 Compare May 15, 2021 22:36
@sipa
Copy link
Collaborator Author

sipa commented May 16, 2021

This is ready.

@gmaxwell
Copy link
Contributor

utACK 1e96b67 (I also tested it, but not with mingw or msvc, so it doesn't count)

@sipa sipa merged commit 4c1d32b into master May 16, 2021
@gmaxwell
Copy link
Contributor

okay, I tested mingw32 too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants