Develop/holland01 #7
Conversation
base58.c
Outdated
| #ifdef _WIN64 | ||
| typedef int64_t ssize_t; | ||
| #else | ||
| typedef int32_t ssize_t; |
There was a problem hiding this comment.
Would prefer to just avoid ssize_t (use size_t and avoid circumstances that can lead to a negative value) rather than try to typedef it. Pretty sure this will break on Mingw.
There was a problem hiding this comment.
I'm honestly wondering why ssize_t existed there in the first place, then?
There was a problem hiding this comment.
Convenience; j can get to -1 with the current code. So we need to handle it differently to avoid that.
There was a problem hiding this comment.
Moved this issue to #8; please review
base58.c
Outdated
| #include <arpa/inet.h> | ||
| #else | ||
| #include <winsock2.h> | ||
| #include <malloc.h> |
There was a problem hiding this comment.
There doesn't seem to be any malloc.h on at least Mingw cross-compilers.
There was a problem hiding this comment.
I'll replace with an appropriate _MSC_VER check.
| #endif | ||
|
|
||
| #include <stdbool.h> | ||
| #include <stddef.h> | ||
| #include <stdint.h> |
There was a problem hiding this comment.
We still need stdint.h here, as this file uses intN_t types.
There was a problem hiding this comment.
Yes, and it's included in the header file
There was a problem hiding this comment.
It should be included in files where it is used directly.
| @@ -5,19 +5,31 @@ | |||
| * under the terms of the standard MIT license. See COPYING for more details. | |||
| */ | |||
|
|
|||
| #ifndef WIN32 | |||
| #include "libbase58.h" | |||
There was a problem hiding this comment.
stdint.h is in libbase58.h - see here.
stdint.h is needed in the header for msvc compilation.
I also didn't see any reason include 4 header files before the libbas58.h file, considering that most of those aren't necessary for the function declarations found in it.
base58.c
Outdated
| @@ -37,39 +49,69 @@ typedef uint32_t b58_almostmaxint_t; | |||
| #define b58_almostmaxint_bits (sizeof(b58_almostmaxint_t) * 8) | |||
| static const b58_almostmaxint_t b58_almostmaxint_mask = ((((b58_maxint_t)1) << b58_almostmaxint_bits) - 1); | |||
|
|
|||
| // MSVC 2017 doesn't support the GCC (and probably clang) extension | |||
| // for dynamic arrays in C | |||
There was a problem hiding this comment.
Variable length arrays are a standard C99 feature, not an extension. The library is intentionally designed to not use malloc/calloc/free.
There was a problem hiding this comment.
Variable length arrays are a standard C99 feature, not an extension. The library is intentionally designed to not use malloc/calloc/free.
That may be, but MSVC still doesn't support them. _alloca or the MSVC _malloca can be used if that's preferred - these perform stack allocation (though _malloca will allocate heap for sufficiently large sizes).
There was a problem hiding this comment.
AFAIK, MSVC is a C++ compiler, not a C compiler.
I suppose it can't hurt to use _malloca or such, based on a failed configure check for standards compatibility...
There was a problem hiding this comment.
A few years back they added C99 support. It's still subpar in comparison to MinGW/GCC, but it's better than it used to be - enough to where people who like C end up using it over C++.
There was a problem hiding this comment.
Wrote a simple alternative in #9; can you check if it works?
| b58_maxint_t t; | ||
| b58_almostmaxint_t c; | ||
| size_t i, j; | ||
| uint8_t bytesleft = binsz % sizeof(b58_almostmaxint_t); | ||
| b58_almostmaxint_t zeromask = bytesleft ? (b58_almostmaxint_mask << (bytesleft * 8)) : 0; | ||
| unsigned zerocount = 0; | ||
|
|
There was a problem hiding this comment.
Please don't mess with the formatting.
There was a problem hiding this comment.
Apologies, I'll be sure to omit any of those in the future.
Detects j dropping below 0 going unchecked. This bug was originally fixed in 2c6b791, but due to ssize_t being a POSIX extension, we are going to check it manually.
Only j could become negative, so we simply check before it would
|
@luke-jr Ok, changes have been added. Please review |
base58.c
Outdated
| #define b58_log_err(msg, ...) printf("[" __FUNCTION__ " - ERROR]: " msg, __VA_ARGS__) | ||
|
|
||
| #define b58_alloc_mem(type, name, count) \ |
There was a problem hiding this comment.
I'm honestly not sure what's up with the indentation of the escape slashes here. With invisibles in Atom turned on everything looks as if it's tabbed appropriately, and this is exactly how it looked before the push:
|
@luke-jr 🏓 What is needed to merge this PR and fix the Windows build? |
|
At the very least, a clean and reviewable branch... |
Just support for MSVC. Note that ssize_t is a posix extension which Windows doesn't appear to support still.