[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. In D130516#3710972 , @MaskRay wrote: > I have only taken very brief look at the new version. Having an enum class > `CompressionKind` with a parallel `CompressionAlgorithm` seems redundant. > `friend CompressionAlgorithm *Compre

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. In D130516#3710903 , @ckissane wrote: > In D130516#3703691 , @dblaikie > wrote: > >> This is looking pretty close to what I've been picturing - the only thing >> remaining is that I thi

[PATCH] D130516: [llvm] compression classes

MaskRay added a comment. I have only taken very brief look at the new version. Having an enum class `CompressionKind` with a parallel `CompressionAlgorithm` seems redundant. `friend CompressionAlgorithm *CompressionKind::operator->() const;` looks magical. I hope that someone insisting on objec

[PATCH] D130516: [llvm] compression classes

ckissane added a comment. In D130516#3703691 , @dblaikie wrote: > This is looking pretty close to what I've been picturing - the only thing > remaining is that I think we could get rid of `CompressionKind` and access > the `CompressionAlgorithm` directl

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 451247. ckissane added a comment. - address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clangd/index/Serialization.cpp

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. This is looking pretty close to what I've been picturing - the only thing remaining is that I think we could get rid of `CompressionKind` and access the `CompressionAlgorithm` directly - basically the contents of `CompressionKind::operator->` could be a free/public fun

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 450401. ckissane added a comment. - Merge remote-tracking branch 'origin/main' into ckissane.compression-class-simple Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 450392. ckissane added a comment. - fix InputSection decompress issue Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clangd/index/Seriali

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 450345. ckissane added a comment. - format error string Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clangd/index/Serialization.cpp c

[PATCH] D130516: [llvm] compression classes

ckissane added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:1474-1475 + if (!CompressionScheme) { +Error("compression class " + + (CompressionScheme->Name + " is not available").str()); return nullptr; leon

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 450343. ckissane added a comment. - remove OptionalCompressionKind noneIfUnsupported Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clang

[PATCH] D130516: [llvm] compression classes

ckissane added inline comments. Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:54-60 + OptionalCompressionScheme = compression::noneIfUnsupported( + (Compress && DoInstrProfNameCompression) ? OptionalCompressionScheme +

[PATCH] D130516: [llvm] compression classes

ckissane added inline comments. Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:54-60 + OptionalCompressionScheme = compression::noneIfUnsupported( + (Compress && DoInstrProfNameCompression) ? OptionalCompressionScheme +

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 450338. ckissane added a comment. - cleanup some compression nits Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clangd/index/Serializati

[PATCH] D130516: [llvm] compression classes

dblaikie added inline comments. Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:54-60 + OptionalCompressionScheme = compression::noneIfUnsupported( + (Compress && DoInstrProfNameCompression) ? OptionalCompressionScheme +

[PATCH] D130516: [llvm] compression classes

ckissane added inline comments. Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:54-60 + OptionalCompressionScheme = compression::noneIfUnsupported( + (Compress && DoInstrProfNameCompression) ? OptionalCompressionScheme +

[PATCH] D130516: [llvm] compression classes

ckissane added inline comments. Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:54-60 + OptionalCompressionScheme = compression::noneIfUnsupported( + (Compress && DoInstrProfNameCompression) ? OptionalCompressionScheme +

[PATCH] D130516: [llvm] compression classes

ckissane added inline comments. Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:54-60 + OptionalCompressionScheme = compression::noneIfUnsupported( + (Compress && DoInstrProfNameCompression) ? OptionalCompressionScheme +

[PATCH] D130516: [llvm] compression classes

dblaikie added inline comments. Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:54-60 + OptionalCompressionScheme = compression::noneIfUnsupported( + (Compress && DoInstrProfNameCompression) ? OptionalCompressionScheme +

[PATCH] D130516: [llvm] compression classes

leonardchan added inline comments. Comment at: clang-tools-extra/clangd/index/Serialization.cpp:255-256 +} else + return error("Compressed string table, but " + + (CompressionScheme->Name + " is unavailable").str()); + } nit: I think `

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 450176. ckissane added a comment. - move if into switch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clangd/index/Serialization.cpp c

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 450169. ckissane edited the summary of this revision. ckissane added a comment. - fix some nits in Compression.h Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 File

[PATCH] D130516: [llvm] compression classes

ckissane added a comment. @dblaikie @MaskRay I would like it if you could all take another look. In response to @dblaikie 's comments about implementation weight I have greatly simplified the implementation, including removing extra capitalized function overloads (Compress, Decompress), and rem

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 450093. ckissane added a comment. - remove compression kind || && overload Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clangd/index/Se

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 449772. ckissane added a comment. - remove uppercase Compress, Decompress Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clangd/index/Ser

[PATCH] D130516: [llvm] compression classes

ckissane added inline comments. Comment at: llvm/include/llvm/Support/Compression.h:48-51 + Error decompress(ArrayRef Input, uint8_t *UncompressedBuffer, + size_t &UncompressedSize) { +return Decompress(Input, UncompressedBuffer, UncompressedSize); + } ---

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. > Yes, I can continue to trim down the implementation! I agree with your > sentiment. Thanks! This update helps - though I think we'll still want to further isolate the different pieces of this change/reduce this further. > I agree with some of this, I have some stron

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 449421. ckissane added a comment. - compression api: greatly simplify implementation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clang

[PATCH] D130516: [llvm] compression classes

ckissane added a comment. In D130516#3694422 , @dblaikie wrote: > The current code here still seems more complicated than I'd prefer - looks > like currently the size/speed/default levels are currently unused, so maybe > we can omit those for now, knowi

[PATCH] D130516: [llvm] compression classes

MaskRay added a comment. In D130516#3694366 , @dblaikie wrote: > In D130516#3694151 , @MaskRay wrote: > >> In D130516#3688236 , @dblaikie >> wrote: >> >>> In D130516#3688

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. > I think I have worked out something that is the best of both worlds: I think @MaskRay's main concern, which I share to a degree, is that there's a lot of code/complexity here that doesn't currently seem warranted by the size of the problem. So adding more implementat

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. The current code here still seems more complicated than I'd prefer - looks like currently the size/speed/default levels are currently unused, so maybe we can omit those for now, knowing they will be added? And the CompressionKind with all its operator overloads seems li

[PATCH] D130516: [llvm] compression classes

ckissane added a comment. In D130516#3694366 , @dblaikie wrote: > In D130516#3694151 , @MaskRay wrote: > >> In D130516#3688236 , @dblaikie >> wrote: >> >>> In D130516#368

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 449375. ckissane added a comment. - CompressionKind: clean up param names to == op Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clangd/

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. In D130516#3694151 , @MaskRay wrote: > In D130516#3688236 , @dblaikie > wrote: > >> In D130516#3688123 , @MaskRay >> wrote: >> >>> I'd like to

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 449365. ckissane added a comment. - trim down compression api: remove supported() Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clangd/i

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 449355. ckissane added a comment. - make a zlib corruption check specific Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clangd/index/Ser

[PATCH] D130516: [llvm] compression classes

MaskRay added a comment. In D130516#3688236 , @dblaikie wrote: > In D130516#3688123 , @MaskRay wrote: > >> I'd like to make a few arguments for the current namespace+free function >> design, as opposed to the cla

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 449346. ckissane added a comment. - feat compression "enum" with methods Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clangd/index/Seri

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. (still lots of outstanding comments from my last round, so I won't reiterate those - but waiting for some responses to them) Comment at: llvm/lib/Support/Compression.cpp:30-32 +ZStdCompressionAlgorithm +*llvm::compression::ZStdCompressionAlgorithm

[PATCH] D130516: [llvm] compression classes

ckissane added inline comments. Comment at: llvm/lib/Support/Compression.cpp:30-32 +ZStdCompressionAlgorithm +*llvm::compression::ZStdCompressionAlgorithm::Instance = +new ZStdCompressionAlgorithm(); dblaikie wrote: > leonardchan wrote: > > Perhaps fo

[PATCH] D130516: [llvm] compression classes

dblaikie added a comment. In D130516#3688123 , @MaskRay wrote: > I'd like to make a few arguments for the current namespace+free function > design, as opposed to the class+member function design as explored in this > patch (but thanks for the exploratio

[PATCH] D130516: [llvm] compression classes

leonardchan added inline comments. Comment at: llvm/lib/Support/Compression.cpp:30-32 +ZStdCompressionAlgorithm +*llvm::compression::ZStdCompressionAlgorithm::Instance = +new ZStdCompressionAlgorithm(); Perhaps for each of these, you could instead hav

[PATCH] D130516: [llvm] compression classes

MaskRay added a comment. I'd like to make a few arguments for the current namespace+free function design, as opposed to the class+member function design as explored in this patch (but thanks for the exploration!). Let's discuss several use cases. (a) if a use case just calls compress/uncompress

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 448473. ckissane added a comment. - fix usage of CompressionAlgorithmFromId Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clangd/index/S

[PATCH] D130516: [llvm] compression classes

ckissane updated this revision to Diff 448469. ckissane added a comment. - make compression singletons Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 Files: clang-tools-extra/clangd/index/Serialization

[PATCH] D130516: [llvm] compression classes

leonardchan added inline comments. Comment at: clang-tools-extra/clangd/index/Serialization.cpp:193-194 } -if (llvm::compression::zlib::isAvailable()) { +llvm::compression::CompressionAlgorithm *CompressionScheme = +new llvm::compression::ZlibCompressionAlgor

[PATCH] D130516: [llvm] compression classes

dblaikie added inline comments. Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:50-58 + compression::CompressionAlgorithm *CompressionScheme = + new compression::ZlibCompressionAlgorithm(); + + CompressionScheme = + CompressionScheme->when(Compress

[PATCH] D130516: [llvm] compression classes

ckissane marked 2 inline comments as done. ckissane added a comment. marked outdated comments as done Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130516/new/ https://reviews.llvm.org/D130516 ___ cfe-co