sipa / minisketch

Minisketch: an optimized library for BCH-based set reconciliation
MIT License
310 stars 52 forks source link

A few improvements to prepare for subtree in Bitcoin Core #36

Closed sipa closed 3 years ago

sipa commented 3 years ago

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

sipa commented 3 years ago

cc @sipsorcery

gmaxwell commented 3 years ago

I believe _WIN32 is the standard rather than WIN32.

sipsorcery commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

gmaxwell commented 3 years ago

utACK 4721111ca4f50b5137b721bb04e97fcdd0c35984 this should be tested by someone on MSVC.

sipa commented 3 years ago

I'm testing this in https://github.com/bitcoin/bitcoin/pull/21859, so still making changes.

sipa commented 3 years ago

This is ready.

gmaxwell commented 3 years ago

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

gmaxwell commented 3 years ago

okay, I tested mingw32 too.