[PATCH] D128754: Refactor LLVM compression namespaces

[Leonard Chan via Phabricator via cfe-commits]

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