[PATCH] D128667: [WIP] Add Zstd ELF support

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:567-568
+    compression::zstd::compress(
+        StringRef(reinterpret_cast<const char *>(OriginalData.data()),
+                  OriginalData.size()),
+        CompressedData);
----------------
This StringRef construction is identical in both parts of the if, so should be pulled out of the if statement into a local variable.


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOptions.cpp:1009-1019
+  if (Config.CompressionType != DebugCompressionType::Zstd) {
+    if (Config.DecompressDebugSections && !compression::zlib::isAvailable())
+      return createStringError(
+          errc::invalid_argument,
+          "LLVM was not compiled with LLVM_ENABLE_ZLIB: cannot decompress");
+  } else {
+    if (Config.DecompressDebugSections && !compression::zstd::isAvailable())
----------------
This is identical (I think?) to a block further up in the file. Could we pull it out into a helper function, please? Something like the following:

```
Error checkCompressionAvailability() {
   /* moved code here */
}

// Usage would look like:
if (Error err = checkCompressionAvailability())
  return err;
```


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

https://reviews.llvm.org/D128667