Page MenuHomePhabricator

[llvm] Remove unused and redundant crc32 funcction from llvm::compression::zlib namespace
ClosedPublic

Authored by ckissane on Jun 28 2022, 12:56 PM.

Details

Summary
  • Remove crc32 from zlib compression namespace, people should use the llvm::crc32 instead.

Diff Detail

Event Timeline

ckissane created this revision.Jun 28 2022, 12:56 PM
ckissane requested review of this revision.Jun 28 2022, 12:56 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 28 2022, 12:56 PM
ckissane updated this revision to Diff 440736.Jun 28 2022, 12:57 PM

up release notes

ckissane edited the summary of this revision. (Show Details)Jun 28 2022, 2:05 PM

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.

clang-tools-extra/clangd/index/Serialization.cpp
242 ↗(On Diff #440736)

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".

llvm/unittests/Support/CompressionTest.cpp
69

Should this be replaced with llvm::crc32 rather than deleting this?

ckissane marked an inline comment as done.Jun 28 2022, 7:37 PM
ckissane added inline comments.
llvm/unittests/Support/CompressionTest.cpp
69

No Because llvm::crc32 has it's own tests already

ckissane marked an inline comment as done.Jun 28 2022, 7:40 PM
ckissane added inline comments.
clang-tools-extra/clangd/index/Serialization.cpp
242 ↗(On Diff #440736)

sure, I can add a AlgorithmName property to the zlib namespace and future compression namespaces, then we can print out compression::serialize::AlgorithmName to see what is being used

ckissane updated this revision to Diff 441101.Jun 29 2022, 11:28 AM

add AlgorithmName to compression namespaces

ckissane marked an inline comment as done.Jun 29 2022, 11:37 AM

Marked name of algorithm comments as done

ckissane updated this revision to Diff 441107.Jun 29 2022, 11:41 AM
  • feat: refactor llvm compression namespaces
  • chore: update rel notes to note zlib::crc32 remove
  • feat: add AlgorithmName to compression namespaces
ckissane updated this revision to Diff 441113.Jun 29 2022, 12:03 PM
  • Merge remote-tracking branch 'origin/main' into ckissane.refactor-compression-namespace
leonardchan added inline comments.Jun 29 2022, 3:32 PM
llvm/docs/ReleaseNotes.rst
211–219

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.

llvm/lib/Support/Compression.cpp
29

Should createError still be wrapped with #if LLVM_ENABLE_ZLIB?

MaskRay added inline comments.Jun 29 2022, 3:55 PM
llvm/docs/ReleaseNotes.rst
211–219

I agree with this plan. Changes like zlib_unavailable need to bring attention to the usual contributors/reviewers in the area.

llvm/lib/Support/Compression.cpp
27
llvm/unittests/Support/CompressionTest.cpp
23
ckissane updated this revision to Diff 441537.Jun 30 2022, 3:11 PM

make part 1 after part 0 of refactor

ckissane retitled this revision from Refactor LLVM compression namespaces to Refactor LLVM compression namespaces (Part 1: remove crc32).Jun 30 2022, 3:12 PM
ckissane edited the summary of this revision. (Show Details)Jun 30 2022, 3:12 PM
ckissane updated this revision to Diff 441541.Jun 30 2022, 3:23 PM
  • Merge branch 'ckissane.refactor-compression.part-0' into ckissane.refactor-compression.part-1
leonardchan accepted this revision.Jun 30 2022, 3:26 PM

LGTM. Should probably add [llvm] to the subject also. Generally it's helpful for immediately knowing which subrepo you're touching.

This revision is now accepted and ready to land.Jun 30 2022, 3:26 PM
ckissane retitled this revision from Refactor LLVM compression namespaces (Part 1: remove crc32) to [llvm] Refactor LLVM compression namespaces (Part 1: remove crc32).Jul 1 2022, 12:10 PM
ckissane marked 3 inline comments as done.

addressed comments marked done

ckissane updated this revision to Diff 441770.Jul 1 2022, 12:13 PM
ckissane edited the summary of this revision. (Show Details)

updated diff

ckissane retitled this revision from [llvm] Refactor LLVM compression namespaces (Part 1: remove crc32) to [llvm] Remove unused and redundant crc32 funcction from llvm::compression::zlib namespace.Jul 1 2022, 12:15 PM
MaskRay accepted this revision.Jul 1 2022, 1:34 PM

Thanks!

llvm/lib/Support/Compression.cpp
24–25

if a previous patch introduces a blank line between two using. please drop it from the previous patch.

llvm/unittests/Support/CompressionTest.cpp
20–21

if a previous patch introduces a blank line between two using. please drop it from the previous patch.

ckissane updated this revision to Diff 443009.Jul 7 2022, 11:45 AM
ckissane edited the summary of this revision. (Show Details)
  • remove crc32 from zlib compression namespace
  • Merge branch 'ckissane.refactor-compression.part-0' into ckissane.refactor-compression.part-1
  • Merge branch 'ckissane.refactor-compression.part-0.no-string-sub' into ckissane.refactor-compression.part-1
  • Merge remote-tracking branch 'origin/ckissane.refactor-compression.part-0.no-string-sub' into ckissane.refactor-compression.part-1
ckissane marked 4 inline comments as done.Jul 7 2022, 11:46 AM

mark handled comments as done

MaskRay accepted this revision.Jul 7 2022, 2:19 PM

You may push this patch and its prerequisite while other patches are still to be reviewed.