[PATCH] D128754: Refactor LLVM compression namespaces

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: llvm/docs/ReleaseNotes.rst:183-191
+* Refactor compression namespaces across the project, making way for a possible
+  introduction of alternatives to zlib compression in the llvm toolchain.
+  Changes are as follows:
+  * Remove crc32 from zlib compression namespace, people should use the ``llvm::crc32`` instead.
+  * Relocate the ``llvm::zlib`` namespace to ``llvm::compression::zlib``.
+  * Code that explictly needs ``zlib`` compression (IE zlib elf debug sections) should use ``llvm::compression::zlib``.
+  * Code interfacing with compressed profile data should use ``llvm::compression::profile``.
----------------
leonardchan wrote:
> I think maybe even more of these could be split into further patches. I would expect that this patch contain just the namespace refactoring but not necessarily any code deletion or cmake changes. I think this could be:
> 
> - One patch that's just the namespace changes; anything else like variable name changes or string changes would be in separate patches, then this patch would be pure RFC and can get a quick LGTM
>   - One followup patch to this that adds `AlgorithmName` to the nested `zlib` namespace.
> - One patch that removes crc32 (and its test)
> - One patch for cmake changes
> - One patch for the `zlib_unavailable -> compression_unavailable` change and logging string changes in profdata
> 
> And if necessary, each of them would have an appropriate ReleaseNodes.rst update.
I agree with this plan. Changes like `zlib_unavailable` need to bring attention to the usual contributors/reviewers in the area.


================
Comment at: llvm/lib/Support/Compression.cpp:26
 
-#if LLVM_ENABLE_ZLIB
+using namespace compression;
+
----------------



================
Comment at: llvm/unittests/Support/CompressionTest.cpp:22
 
+using namespace compression;
+
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128754/new/

https://reviews.llvm.org/D128754