aklomp / base64

Fast Base64 stream encoder/decoder in C99, with SIMD acceleration
BSD 2-Clause "Simplified" License
865 stars 162 forks source link

fix: Detect platform word size based on `SIZE_MAX` #125

Closed BurningEnlightenment closed 7 months ago

BurningEnlightenment commented 9 months ago

The native word size on a platform matches the size of size_t with a few rare exceptions like the x32 ABI on linux. Therefore we can derive a general cross platform value for BASE64_WORDSIZE based on SIZE_MAX which the C99 standard guarantees to be available and well defined.

This fixes the misdection of x86/arm32 win32 as a 64bit platform, i.e. it is an alternative to #123 @mayeut I think this should also cover the __WORDSIZE and __SIZE_WIDTH__ cases. Can you verify this? If that indeed is the case I would like remove those branches, too.

See also https://en.cppreference.com/w/c/types/limits

mayeut commented 7 months ago

@BurningEnlightenment, I did not write anything related to __WORDSIZE. For __SIZE_WIDTH__, it was directly as bit-size for Alpine but indeed, using SIZE_MAX is equivalent.

BurningEnlightenment commented 7 months ago

@mayeut sorry w.r.t. the __WORDSIZE misattribution--the git-blame was a bit misleading due to 955e9addde55d0fb4d1c0a56cb5a95c48c5bb16a. Apparently the __WORDSIZE usage goes all the way back to 41b1b7750d1a809b6ee9da0650622112f8fbd165. So I will go ahead and remove those branches.

aklomp commented 7 months ago

Merged with the fixes proposed by @mayeut.

@BurningEnlightenment I slightly modified the commit title to add the issue number, and I modified the commit body to mark this issue and #123 as resolved. Hope you don't mind.

BurningEnlightenment commented 7 months ago

@aklomp I additionally wanted to remove these four lines as they became redundant with this change https://github.com/aklomp/base64/blob/88607feedaa57abd382d6ceb4b296629c6626f08/lib/env.h#L51-L54 should I open a new PR?

aklomp commented 7 months ago

@BurningEnlightenment I think it's a bit too late now to force-push to the master branch, so yes, please open a new PR.