C++: Make LZ4 and ZSTD dependencies optional + clang-format #1008
Conversation
|
|
| #ifndef MCAP_COMPRESSION_NO_ZSTD | ||
| case Compression::Zstd: | ||
| decompressor = static_cast<ICompressedReader*>(&zstdReader_); | ||
| break; |
There was a problem hiding this comment.
An MCAP chunk containing an unrecognized compression string will cause a reader to defereference a null pointer
here. Before this patch we would have assumed any unrecognized encoding was zstd, which would have caused some confusing error message, but likely not a segfault.
Would you be able to add an early return here in case of an unrecognized compression method?
There was a problem hiding this comment.
You mean decompressor pointer? It is initialized to uncompressed reader in the default switch case above.
There was a problem hiding this comment.
I've modified it slightly to make behavior the same as before when ZSTD is enabled.
There was a problem hiding this comment.
I don't think it makes sense to default to zstd here, we should set status_ and return.
When both lz4 and zstd are built into the reader, this switch case is exhaustive. This is why the existing code works: we detected unrecognised compression strings in ParseCompression before ever getting here.
We're introducing a new error condition here for when a compression string is recognised but not supported. I think it makes sense to add a new error in errors.hpp for this. We could then go through all the supported compression values in this switch statement, and in the default: case set status_ = and return early.
There was a problem hiding this comment.
Ok, now ParseCompression should fail if compression is not compiled in.
New defines: MCAP_COMPRESSION_NO_ZSTD, MCAP_COMPRESSION_NO_LZ4
|
Thanks for the PR! I'm continuing with your changes here: #1013 |
…1013) ### Public-Facing Changes Adds two new build-time flags to optionally disable `zstd` and `lz4` support. These are `MCAP_COMPRESSION_NO_ZSTD` and `MCAP_COMPRESSION_NO_LZ4`. When reading MCAP files, if the reader encounters a known compression method but is compiled without support for it, it will yield `Status::UnsupportedCompression`. ### Description Based on #1008 A new test binary is added to CI, which exercises these build-time flags. --------- Co-authored-by: Alexander Sherikov <[email protected]>
…oxglove#1013) ### Public-Facing Changes Adds two new build-time flags to optionally disable `zstd` and `lz4` support. These are `MCAP_COMPRESSION_NO_ZSTD` and `MCAP_COMPRESSION_NO_LZ4`. When reading MCAP files, if the reader encounters a known compression method but is compiled without support for it, it will yield `Status::UnsupportedCompression`. ### Description Based on foxglove#1008 A new test binary is added to CI, which exercises these build-time flags. --------- Co-authored-by: Alexander Sherikov <[email protected]>
Public-Facing Changes
New defines: MCAP_COMPRESSION_NO_ZSTD, MCAP_COMPRESSION_NO_LZ4
Description
Make LZ4 and ZSTD dependencies optional