[PATCH] D130458: [llvm-objcopy] Support --{,de}compress-debug-sections for zstd

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.h:539
 
+  uint32_t OrigChType = 0;
   DebugCompressionType CompressionType;
----------------
There's only one `ChType` in this context, not an original and a modified one, so I'm not sure we need the `Orig` prefix? (Also applies to the getter obviously)


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/compress-debug-sections-zstd.test:2
+# REQUIRES: zstd
+## Test --compress-debug-sections=zstd and uncompression.
+
----------------
To me, "decompression" sounds much more natural than "uncompression" (FWIW, I'm not sure I'd ever use the verb "uncompress").

Later on, `UNCOMPRESSED` should probably be the more specific `DECOMPRESSED` (see https://english.stackexchange.com/a/56483), since it's a reference to something that has undergone the reverse of compression.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/compress-debug-sections-zstd.test:29-30
+
+## --compress-debug-sections does not update a compressed section. Its compression
+## type does not change.
+# RUN: llvm-objcopy --compress-debug-sections=zstd %t-zstd %t-zstd-zstd
----------------
This comment makes me think it might be good to have a test case that shows we don't convert a zstd section to a zlib section if specified with `--compress-debug-sections=zlib` and vice versa.


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:743
+            errc::invalid_argument,
+            "LLVM was not compiled with LLVM_ENABLE_ZSTD: can not compress");
+      }
----------------
FWIW, it should be "cannot", to conform to modern English trends ("can not" means the same, but is much less common).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130458