5 ms·

This looks like a nice and concise library. Single header file, many useful functions, and a usable license. The only thing I really dislike are all of the type

by szanni 6y ago

This looks like a nice and concise library. Single header file, many useful functions, and a usable license. The only thing I really dislike are all of the typedefs. zpl_u64 zpl_crc64(void const *data, zpl_isize len); Const correctness, good! But why is the size parameter signed? Why not use size_t and uint64_t like the C stdlib?

messe 6y ago
The zpl_u64 seems to be because they're trying to support older versions of the MSVC compiler[1]. As for why they're using a signed size parameter, I'm not really sure. One idiom they appear to use often is[2]: for (...; x--; y++) where x is a size-type of some kind. They may be defaulting to signed size types to avoid errors in case somebody decides to change to above to: for (...; x-- >= 0; y++) There's even a macro zpl_size_of: #define zpl_size_of(x) (zpl_isize)(sizeof(x)) But the real weirdness is in the other macros: #define cast(Type) (Type) ...just why? And #define ZPL_MULTILINE(...) #__VA_ARGS__ is supposed to be a way of having multiline literals in C. But, everything inside the "..." has to be valid C tokens. It might work as a string literal in many cases, but it's misleading at best and broken at worst. I'm not sure what the advantage of the following is vs. just setting the bits using & and | which every C programmer should already be familiar with: #define ZPL_MASK_SET(var, set, mask) \ do { \ if (set) \ (var) |= (mask); \ else \ (var) &= ~(mask); \ } while (0) There's surely no excuse for this: #define zpl_global static // Global variables While a lot of the library looks useful, stuff like the above makes it feel like it's written by somebody who knows C, but doesn't feel fully comfortable writing it. [1]: https://github.com/zpl-c/zpl/blob/master/code/header/essentials/types.h#L11-L40 https://github.com/zpl-c/zpl/blob/master/code/header/essenti... [2]: https://github.com/zpl-c/zpl/blob/27c80bd5807da5238777bfcba8588731b77683cc/code/source/hashing.c#L136 https://github.com/zpl-c/zpl/blob/27c80bd5807da5238777bfcba8...
- up2isomorphism 6y ago
  These extra wrappers are most likely when library writer trying to cover windows platform, which has almost never been a good place for ANSI C programming.
- keldaris 6y ago
  > There's surely no excuse for this The "excuse" is greppability since static has three completely different meanings in C and the desire to clearly denote global variables is fairly common. The same reasoning goes for the cast define as well. Whether you find value in these small abuses of the preprocessor depends on your coding habits, but I don't really see much reason to criticize them. If the author finds them helpful, what's the harm? There isn't much room for confusion and they're not forced on the users of the library.
badsectoracula 6y ago
From a quick look at the code it seems that zpl_isize is used to represent indices, counts, etc and in those cases -1 is often used as an "invalid" value with special meaning (e.g. in the hashtable -1 is used as a "not found" index).
h_anna_h 6y ago
A bit sad that they did not go for uint64_t zpl_crc64(size_t len, const unsigned char data[len]);
- colejohnson66 6y ago
  Is that even portable?
  - kiwidrew 6y ago
    It's a C99 variable-length array declaration, so the syntax is valid; but unfortunately it doesn't actually do anything useful. The 'const unsigned char data[len]' declaration just decays to 'const unsigned char *data' -- the first level of array-ness is discarded. [ there was a bit of discussion about this earlier in the month: https://news.ycombinator.com/item?id=26349903 https://news.ycombinator.com/item?id=26349903 ]
    - h_anna_h 6y ago
      It is useful for static analysis, even the standard people say that they will move into this kind of declaration. > 15. Application Programming Interfaces (APIs) should be self-documenting when possible. In particular, the order of parameters in function declarations should be arranged such that the size of an array appears before the array. The purpose is to allow Variable-Length Array (VLA) notation to be used. This not only makes the code's purpose clearer to human readers, but also makes static analysis easier. Any new APIs added to the Standard should take this into consideration. from http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2086.htm http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2086.htm
    - pjmlp 6y ago
      Which was dropped in C11, with clang and gcc probably being the only C compilers on Earth that might still support it on C11 and C17 mode.
      - h_anna_h 6y ago

by szanni 6y ago

messe 6y ago

The zpl_u64 seems to be because they're trying to support older versions of the MSVC compiler[1]. As for why they're using a signed size parameter, I'm not really sure. One idiom they appear to use often is[2]: for (...; x--; y++) where x is a size-type of some kind. They may be defaulting to signed size types to avoid errors in case somebody decides to change to above to: for (...; x-- >= 0; y++) There's even a macro zpl_size_of: #define zpl_size_of(x) (zpl_isize)(sizeof(x)) But the real weirdness is in the other macros: #define cast(Type) (Type) ...just why? And #define ZPL_MULTILINE(...) #__VA_ARGS__ is supposed to be a way of having multiline literals in C. But, everything inside the "..." has to be valid C tokens. It might work as a string literal in many cases, but it's misleading at best and broken at worst. I'm not sure what the advantage of the following is vs. just setting the bits using & and | which every C programmer should already be familiar with: #define ZPL_MASK_SET(var, set, mask) \ do { \ if (set) \ (var) |= (mask); \ else \ (var) &= ~(mask); \ } while (0) There's surely no excuse for this: #define zpl_global static // Global variables While a lot of the library looks useful, stuff like the above makes it feel like it's written by somebody who knows C, but doesn't feel fully comfortable writing it. [1]: https://github.com/zpl-c/zpl/blob/master/code/header/essentials/types.h#L11-L40 https://github.com/zpl-c/zpl/blob/master/code/header/essenti... [2]: https://github.com/zpl-c/zpl/blob/27c80bd5807da5238777bfcba8588731b77683cc/code/source/hashing.c#L136 https://github.com/zpl-c/zpl/blob/27c80bd5807da5238777bfcba8...

up2isomorphism 6y ago
These extra wrappers are most likely when library writer trying to cover windows platform, which has almost never been a good place for ANSI C programming.
keldaris 6y ago
> There's surely no excuse for this The "excuse" is greppability since static has three completely different meanings in C and the desire to clearly denote global variables is fairly common. The same reasoning goes for the cast define as well. Whether you find value in these small abuses of the preprocessor depends on your coding habits, but I don't really see much reason to criticize them. If the author finds them helpful, what's the harm? There isn't much room for confusion and they're not forced on the users of the library.

badsectoracula 6y ago

From a quick look at the code it seems that zpl_isize is used to represent indices, counts, etc and in those cases -1 is often used as an "invalid" value with special meaning (e.g. in the hashtable -1 is used as a "not found" index).

h_anna_h 6y ago

A bit sad that they did not go for uint64_t zpl_crc64(size_t len, const unsigned char data[len]);

colejohnson66 6y ago
Is that even portable?
- kiwidrew 6y ago
  It's a C99 variable-length array declaration, so the syntax is valid; but unfortunately it doesn't actually do anything useful. The 'const unsigned char data[len]' declaration just decays to 'const unsigned char *data' -- the first level of array-ness is discarded. [ there was a bit of discussion about this earlier in the month: https://news.ycombinator.com/item?id=26349903 https://news.ycombinator.com/item?id=26349903 ]
  - h_anna_h 6y ago
    It is useful for static analysis, even the standard people say that they will move into this kind of declaration. > 15. Application Programming Interfaces (APIs) should be self-documenting when possible. In particular, the order of parameters in function declarations should be arranged such that the size of an array appears before the array. The purpose is to allow Variable-Length Array (VLA) notation to be used. This not only makes the code's purpose clearer to human readers, but also makes static analysis easier. Any new APIs added to the Standard should take this into consideration. from http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2086.htm http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2086.htm
  - pjmlp 6y ago
    Which was dropped in C11, with clang and gcc probably being the only C compilers on Earth that might still support it on C11 and C17 mode.
    - h_anna_h 6y ago