leonardchan added a comment.
> Feedback needed on:
> this namespace approach will result in tooling compression methods being set
> at compile time, however we may want to aim for a runtime configurable
> approach in the future, likely a new revision of the compressed data formats
> that llvm interfaces with with outside tools IE:
>
> - clang ast serialization code could be changed so compressed data could have
> a flag indicating a compression type.
> - profile data code could be changed so compressed data could have a flag
> indicating a compression type as well.
Using the compile time approach for now with the runtime approach in the future
SGTM. I think some flag that could specify the decompression type is what we
want in the long term, but the compile time approach I think is ok for now to
not block you.
================
Comment at: clang-tools-extra/clangd/index/Serialization.cpp:242
+ return error(
+ "Compressed string table, but compression::serialize is unavailable");
----------------
nit: Could we add some function that returns a string of whatever compression
is used? This way we have a more descriptive error message showing what
specifically is unavailable. Same comment elsewhere there's logging/error
reporting but the string is "compression::serialize".
================
Comment at: llvm/unittests/Support/CompressionTest.cpp:68
- 0x414FA339U,
- zlib::crc32(StringRef("The quick brown fox jumps over the lazy dog")));
-}
----------------
Should this be replaced with `llvm::crc32` rather than deleting this?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128754/new/
https://reviews.llvm.org/D128754
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
