[PATCH] D130516: [llvm] compression classes
Leonard Chan via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 4 17:39:30 PDT 2022
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 `error` accepts format-like arguments, so you could have something similar to above with:
```
return error("Compressed string table, but {0} is unavailable", CompressionScheme->Name);
```
================
Comment at: clang/lib/Serialization/ASTReader.cpp:1468-1470
+ if (!OptionalCompressionScheme) {
+ return llvm::MemoryBuffer::getMemBuffer(Blob, Name, true);
+ }
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
================
Comment at: clang/lib/Serialization/ASTReader.cpp:1474-1475
+ if (!CompressionScheme) {
+ Error("compression class " +
+ (CompressionScheme->Name + " is not available").str());
return nullptr;
----------------
I think this `Error` takes a StringRef, so I think you could have:
```
return Error("compression class " + CompressionScheme->Name + " is not available");
```
================
Comment at: lld/ELF/InputSection.cpp:1230
+ if (Error e = compression::CompressionKind::Zlib->decompress(
+ rawData.slice(sizeof(typename ELFT::Chdr)), buf, size))
+ fatal(toString(this) +
----------------
`typename` might not be needed here
================
Comment at: llvm/include/llvm/Support/Compression.h:28
-constexpr int NoCompression = 0;
-constexpr int BestSpeedCompression = 1;
-constexpr int DefaultCompression = 6;
-constexpr int BestSizeCompression = 9;
-
-bool isAvailable();
-
-void compress(ArrayRef<uint8_t> Input,
- SmallVectorImpl<uint8_t> &CompressedBuffer,
- int Level = DefaultCompression);
-
-Error uncompress(ArrayRef<uint8_t> Input, uint8_t *UncompressedBuffer,
- size_t &UncompressedSize);
-
-Error uncompress(ArrayRef<uint8_t> Input,
- SmallVectorImpl<uint8_t> &UncompressedBuffer,
- size_t UncompressedSize);
-
-} // End of namespace zlib
-
-namespace zstd {
-
-constexpr int NoCompression = -5;
-constexpr int BestSpeedCompression = 1;
-constexpr int DefaultCompression = 5;
-constexpr int BestSizeCompression = 12;
-
-bool isAvailable();
-
-void compress(ArrayRef<uint8_t> Input,
- SmallVectorImpl<uint8_t> &CompressedBuffer,
- int Level = DefaultCompression);
-
-Error uncompress(ArrayRef<uint8_t> Input, uint8_t *UncompressedBuffer,
- size_t &UncompressedSize);
-
-Error uncompress(ArrayRef<uint8_t> Input,
- SmallVectorImpl<uint8_t> &UncompressedBuffer,
- size_t UncompressedSize);
-
-} // End of namespace zstd
+struct CompressionAlgorithm {
+ const StringRef Name;
----------------
https://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords since the ctor is non-public
================
Comment at: llvm/include/llvm/Support/Compression.h:40
+ SmallVectorImpl<uint8_t> &CompressedBuffer) {
+ return compress(Input, CompressedBuffer, this->DefaultLevel);
+ }
----------------
`this->` might not be needed here
================
Comment at: llvm/include/llvm/Support/Compression.h:86
+constexpr CompressionKind::operator bool() const {
+ switch (uint8_t(x)) {
+ case uint8_t(CompressionKind::Zlib):
----------------
I think this cast might not be needed
================
Comment at: llvm/include/llvm/Support/Compression.h:106
+ switch (y) {
+ case uint8_t(0):
+ return NoneType();
----------------
I think this cast might not be needed
================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:442-460
+ DebugCompressionType CompressionType =
+ reinterpret_cast<const Elf_Chdr_Impl<ELFT> *>(Sec.OriginalData.data())
+ ->ch_type == ELF::ELFCOMPRESS_ZLIB
+ ? DebugCompressionType::Z
+ : DebugCompressionType::None;
+
+ switch (CompressionType) {
----------------
What's the explanation for having the `llvm_unreachable` branch and getting the compression type? I would've thought this section would just be:
```
if (Error Err1 = compression::CompressionKind::Zlib->decompress(
Compressed, DecompressedContent, static_cast<size_t>(Sec.Size))) {
return createStringError(errc::invalid_argument,
"'" + Sec.Name +
"': " + toString(std::move(Err1)));
}
```
which looks like it would have identical behavior to the old code.
================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:530-536
+ switch (CompressionType) {
+ case DebugCompressionType::Z:
+ compression::CompressionKind::Zlib->compress(OriginalData, CompressedData);
+ break;
+ case DebugCompressionType::None:
+ break;
+ }
----------------
Same here. Should this just be `compression::CompressionKind::Zlib->compress(OriginalData, CompressedData);`? If this is in preparation for the ELF+zstd changes, perhaps we should save those for another patch once that lands?
================
Comment at: llvm/lib/Support/Compression.cpp:53
-bool zlib::isAvailable() { return true; }
-
-void zlib::compress(ArrayRef<uint8_t> Input,
- SmallVectorImpl<uint8_t> &CompressedBuffer, int Level) {
- unsigned long CompressedSize = ::compressBound(Input.size());
- CompressedBuffer.resize_for_overwrite(CompressedSize);
- int Res = ::compress2((Bytef *)CompressedBuffer.data(), &CompressedSize,
- (const Bytef *)Input.data(), Input.size(), Level);
- if (Res == Z_MEM_ERROR)
- report_bad_alloc_error("Allocation failed");
- assert(Res == Z_OK);
- // Tell MemorySanitizer that zlib output buffer is fully initialized.
- // This avoids a false report when running LLVM with uninstrumented ZLib.
- __msan_unpoison(CompressedBuffer.data(), CompressedSize);
- if (CompressedSize < CompressedBuffer.size())
- CompressedBuffer.truncate(CompressedSize);
-}
+ void compress(ArrayRef<uint8_t> Input,
+ SmallVectorImpl<uint8_t> &CompressedBuffer, int Level) {
----------------
nit: add `override`s to be more explicit these are virtual methods
================
Comment at: llvm/lib/Support/Compression.cpp:83-94
+ void compress(ArrayRef<uint8_t> Input,
+ SmallVectorImpl<uint8_t> &CompressedBuffer, int Level) {
+ llvm_unreachable("method:\"compress\" is unsupported for compression "
+ "algorithm:\"zlib\", "
+ "reason:\"llvm not compiled with zlib support\"");
+ };
+ Error decompress(ArrayRef<uint8_t> Input, uint8_t *UncompressedBuffer,
----------------
If the `llvm_unreachable`s should be the default implementation for all subclasses, perhaps the `[de]compress` methods should be regular virtual with these default implementations rather than abstract virtual.
================
Comment at: llvm/lib/Support/Compression.cpp:180-183
+ if (bool(left)) {
+ return left;
+ }
+ return NoneType();
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
or perhaps just
```
return left ? left : NoneType();
```
================
Comment at: llvm/lib/Support/Compression.cpp:188-191
+ if (!left || (!bool(*left))) {
+ return NoneType();
+ }
+ return left;
----------------
Same here
================
Comment at: llvm/lib/Support/Compression.cpp:195
+CompressionAlgorithm *CompressionKind::operator->() const {
+ switch (uint8_t(x)) {
+ case uint8_t(CompressionKind::Zlib):
----------------
I think this cast might not be needed
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130516/new/
https://reviews.llvm.org/D130516
More information about the llvm-commits
mailing list