Detect word size for Windows properly

[zcbenz]

In Windows SDK the _INTEGRAL_MAX_BITS is always 64 even when building for x86_32 targets, so it should not be used for detecting word size when compiling for Windows.

Also _INTEGRAL_MAX_BITS means the max bits of integer, which does not necessarily equal to the word size, so it should be used as the last resort for detecting word size.

Currently compiling Node.js for 32bit Windows with clang breaks because of this.

[aklomp]

The use of the _INTEGRAL_MAX_BITS macro was introduced in fcab175 (by @BurningEnlightenment ) as a way to detect word size on MSVC. This patch will only check that macro on non-Windows systems. Then what is the point of checking the macro? Can't it better be removed entirely if it's not checked on the platform it was introduced for?

[zcbenz]

Thanks for the background, I have removed the usage of _INTEGRAL_MAX_BITS completely.

For compatibility with MSVC, I have added another way to detect word size, which is officially documented.

[BurningEnlightenment]

Maybe I'm missing something, but wouldn't it be possible to add two "generic"/C99 branches based on SIZE_MAX==0xffffffff and SIZE_MAX==0xffffffffffffffff? These should cover the Windows case as there isn't something like x32 on Windows. And as a bonus would make the library a little bit more portable.

[zcbenz]

The purpose of this macro is to conditionally enable code in compile time, I think SIZE_MAX==0xffffffff only works in runtime.

[BurningEnlightenment]

SIZE_MAX is a C99 preprocessor macro, i.e. it is guaranteed to work at compile time / in preprocessor expressions (see cppreference and this simple godbolt-example). The part I'm unsure about is whether SIZE_MAX (resp. sizeof(size_t)) is a good predictor for the wordsize on most machines and therefore whether this would be a good default or not.

[zcbenz]

Thanks for the reference, according to MSDN this trick should work on msvc.

SIZE_MAX | same as _UI64_MAX if _WIN64 is defined, or UINT_MAX

However for other platforms the general definition of SIZE_WIDTH is bit width of object of size_t type, the value of which completely depends on the implementation.

On the other hand _WIN32 and _WIN64 are well documented and there is no room for surprises.

_WIN32 Defined as 1 when the compilation target is 32-bit ARM, 64-bit ARM, x86, or x64. Otherwise, undefined. _WIN64 Defined as 1 when the compilation target is 64-bit ARM or x64. Otherwise, undefined.

[zcbenz]

@aklomp Putting the quest for a generic macro define aside, can you merge this PR and do a patch release first? Node.js updated base64 dep very recently and I would like to fix the broken build soon.

[aklomp]

Hi everyone, sorry for not weighing in on this issue, but unfortunately my availability is very limited right now. I hope to resolve this in one or two weeks at most.

[zcbenz]

Ping @aklomp for a review, I'm good with https://github.com/aklomp/base64/pull/125 too since it fixes my problem anyway.

[aklomp]

@zcbenz Thanks for your patience. I merged #125 instead of this PR because that fix looks a bit more portable by not doing platform detection, and you mentioned that it also solves this issue.

Let me know if you still need a point release. I can do one, but I'd prefer to fix some other issues first (mainly #127).

[zcbenz]

Thanks for looking in this, I'll just wait for a new release so I can update the dep in Node.js.

[aklomp]

@zcbenz I just released v0.5.2.