- Refactor compression namespaces across the project, making way for a possible introduction of alternatives to zlib compression. Changes are as follows:
- Relocate the llvm::zlib namespace to llvm::compression::zlib.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM. Should also probably add [NFC] to the start of the subject
lld/ELF/InputSection.cpp | ||
---|---|---|
77 | ": contains a compressed section, " + "but " -> ": contains a compressed section, but " | |
llvm/lib/ProfileData/InstrProf.cpp | ||
154 | " compression but the profile reader was built " + "without " -> " compression but the profile reader was built without " |
- Merge branch 'ckissane.refactor-compression.part-0' of github.com:ckissane/llvm-project into ckissane.refactor-compression.part-0
- Merge branch 'main' into ckissane.refactor-compression.part-0
llvm/include/llvm/Support/Compression.h | ||
---|---|---|
49–51 | I think we will need to support dynamically selecting (de)compression algorithm for both profile and serialization. For example, we should be able to use read profiles generated by an older version of LLVM that only supported zlib, even if the new one also supports zstd. Given that, I'd omit these and instead use compression::zlib everywhere. |
[NFC] refactor compression namespaces making way for a possible introduction of alternatives to zlib compression in the llvm toolchain.
I'd use [NFC] Refactor llvm::zlib namespace and place supplementary wording to the body.
in the llvm toolchain is redundant in the sentence.
llvm/lib/ProfileData/InstrProf.cpp | ||
---|---|---|
154 | Keep the diagnostic unchanged in this patch. |
llvm/include/llvm/Support/Compression.h | ||
---|---|---|
49–51 | Correct, however I think that doing this in the meantime helps make it clear semantically for what purpose each compression call is for. It will make finding all instances of each time of use much easier in the future, and promotes semantically thought out usage. Therefore I see no reason to not make these explicit aliases as it will only ease transition in the future. |
llvm/lib/ProfileData/InstrProf.cpp | ||
---|---|---|
154 | And the other similar changes? |
I think this can be pushed now. You need to remove the variable
In file included from /var/lib/buildkite-agent/builds/llvm-project/llvm/lib/MC/ELFObjectWriter.cpp:41:
/var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/Support/Compression.h:27:30: error: constexpr variable cannot have non-literal type 'const std::string' (aka 'const basic_string<char>')
static constexpr std::string AlgorithmName = "zlib";
You likely need to run ninja check-llvm check-clang check-clang-tools check-lld check-lldb to ensure all relevant targets still build.
(You can use check-all if your lists of LLVM_ENABLED_PROJECTS/LLVM_ENABLE_RUNTIMES are appropriate.)
llvm/include/llvm/Support/Compression.h | ||
---|---|---|
48 | No need to add blank line between two namespace }. | |
llvm/lib/Support/Compression.cpp | ||
25–27 | delete the excess blank line |
- Merge branch 'ckissane.refactor-compression.part-0' of github.com:ckissane/llvm-project into ckissane.refactor-compression.part-0
- Merge branch 'main' into ckissane.refactor-compression.part-0
- compression refactor: undo string subs
- Merge branch 'ckissane.refactor-compression.part-0' into ckissane.refactor-compression.part-0.no-string-sub
- remove compression namespaace aliases
- update release notes to reflect no compression aliases
- chore: fmt and remove AlgorithmName from zlib namespace
- Merge remote-tracking branch 'origin/main' into ckissane.refactor-compression.part-0.no-string-sub
": contains a compressed section, " + "but " -> ": contains a compressed section, but "