Ideas for API improvements

Originally created by @nemequ on GitHub (Jun 22, 2016). I recently rewrote Squash's brotli extension for the new API. IMHO the API is pretty nice, and I love that it's C not C++. That said, I do have a few ideas for improvements: - [x] For funcions which return 1 on success and 0 on failure, as well as other booleans (like the `is_last` argument to `BrotliEncoderWriteMetaBlock`), please use a `bool` (or `_Bool` if you prefer) intsead of an `int`. Using bool helps make the code more readable and reduce documentation lookups. - [x] The decoder puts the state argument at the end, the encoder puts it first. Please make it consistent. FWIW, I prefer the instance to be first (and that's definitely more common). - [x] I'd really like to see [conformant array parameters](https://www.securecoding.cert.org/confluence/display/c/API05-C.+Use+conformant+array+parameters). This would require a macro [like the one Squash has](https://github.com/quixdb/squash/blob/f4e67dada2fd68d7f1f89b7640a06a8c9a3378ee/squash/squash.h#L52), but it has the potential to help prevent bugs in software using the API, so I think it would be a good addition. I see you already put the length first, so the change is pretty trivial. - [x] It's a bit odd that the everything in the encoder is called `BrotliEncoder*`, but everything in the decoder is `Brotli*` (e.g., `BrotliState` not `BrotliDecoderState`). I think it would be better to move everything in the decoder to `BrotliDecoder*` - [x] Technically, stuff like `_BROTLI_COMMA` isn't allowed. C99 (at least, but IIRC C89 too). Anything which starts with an underscore follewed by an uppercase letter or another underscore. In [C99](http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf), it's in § 7.1.3. I've taken to using an underscore _suffix_ in my code (e.g., `BROTLI_COMMA_`) to indicate something is really supposed to be internal. - [ ] CamelCase for function names is fairly unusual in C. Switching to lowercase_with_underscores would probably be better; CamelCase is usually used for type names (including callbacks like brotli_alloc_func and brotli_free_func). Obviously there is no real standard, but anecdotally it seems like lowercase_with_underscores is the most common… - [ ] Add "zeroed-memory alloc" to memory allocator interface; might improve performance. - [ ] It would be nice to have 'restrict' on the buffers. Obviously it would also have to be hidden behind a macro (feel free to steal [the one from Hedley](https://github.com/nemequ/hedley/blob/master/hedley.h#L311)) - [ ] A lot of parameters can/should be annotated with GCC's [nonnull](https://gcc.gnu.org/onlinedocs/gcc-6.1.0/gcc/Common-Function-Attributes.html#index-g_t_0040code_007bnonnull_007d-function-attribute-3238) attribute (again, [Hedley has a macro](https://github.com/nemequ/hedley/blob/master/hedley.h#L240) you can take). This is great for static analyzers, and if you build with ubsan you can get runtime warnings, too. The macro for this has to be variadic, so you _might_ want to hide this behind a check for C99; variadic macros aren't in C89, but all the common compilers have supported them for a while (even MSVC, since VC8 (2005)). - [ ] `BrotliEncoderMaxCompressedSize` could/should be annotated with the const attribute (GCC ≥ 2.5) or noalias declspec (MSVC, since VC8). Again, [Hedley](https://github.com/nemequ/hedley/blob/master/hedley.h#L302). This one is helpful for optimizing compilers, though honestly I doubt excessive calls to `BrotliEncoderMaxCompressedSize` is a performance bottleneck. These are definitely not major issues, I just wanted to bring them up while changing the API is still an option.

claunia commented

2026-01-29 20:36:09 +00:00

Owner

Originally created by @nemequ on GitHub (Jun 22, 2016).

I recently rewrote Squash's brotli extension for the new API. IMHO the API is pretty nice, and I love that it's C not C++. That said, I do have a few ideas for improvements:

For funcions which return 1 on success and 0 on failure, as well as other booleans (like the is_last argument to BrotliEncoderWriteMetaBlock), please use a bool (or _Bool if you prefer) intsead of an int. Using bool helps make the code more readable and reduce documentation lookups.
The decoder puts the state argument at the end, the encoder puts it first. Please make it consistent. FWIW, I prefer the instance to be first (and that's definitely more common).
I'd really like to see conformant array parameters. This would require a macro like the one Squash has, but it has the potential to help prevent bugs in software using the API, so I think it would be a good addition. I see you already put the length first, so the change is pretty trivial.
It's a bit odd that the everything in the encoder is called BrotliEncoder*, but everything in the decoder is Brotli* (e.g., BrotliState not BrotliDecoderState). I think it would be better to move everything in the decoder to BrotliDecoder*
Technically, stuff like _BROTLI_COMMA isn't allowed. C99 (at least, but IIRC C89 too). Anything which starts with an underscore follewed by an uppercase letter or another underscore. In C99, it's in § 7.1.3. I've taken to using an underscore suffix in my code (e.g., BROTLI_COMMA_) to indicate something is really supposed to be internal.
CamelCase for function names is fairly unusual in C. Switching to lowercase_with_underscores would probably be better; CamelCase is usually used for type names (including callbacks like brotli_alloc_func and brotli_free_func). Obviously there is no real standard, but anecdotally it seems like lowercase_with_underscores is the most common…
Add "zeroed-memory alloc" to memory allocator interface; might improve performance.
It would be nice to have 'restrict' on the buffers. Obviously it would also have to be hidden behind a macro (feel free to steal the one from Hedley)
A lot of parameters can/should be annotated with GCC's nonnull attribute (again, Hedley has a macro you can take). This is great for static analyzers, and if you build with ubsan you can get runtime warnings, too. The macro for this has to be variadic, so you might want to hide this behind a check for C99; variadic macros aren't in C89, but all the common compilers have supported them for a while (even MSVC, since VC8 (2005)).
BrotliEncoderMaxCompressedSize could/should be annotated with the const attribute (GCC ≥ 2.5) or noalias declspec (MSVC, since VC8). Again, Hedley. This one is helpful for optimizing compilers, though honestly I doubt excessive calls to BrotliEncoderMaxCompressedSize is a performance bottleneck.

These are definitely not major issues, I just wanted to bring them up while changing the API is still an option.

claunia referenced this issue

2026-01-29 20:50:36 +00:00

[PR #121] Brotli Bug Fixes #732

Ideas for API improvements #121