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

[Cole Kissane via Phabricator via llvm-commits]

ckissane added a comment.

In D130458#3683370 <https://reviews.llvm.org/D130458#3683370>, @MaskRay wrote:

> Yes!
>
>   compression::Param P;
>   P.Format = CompressionType;
>   compression::compress(P, OriginalData, CompressedData);
>
> is a lightweight use of the compression interface. I could move back the previous version which uses the slightly verbose `switch`, then this patch will have no dependency on D130506 <https://reviews.llvm.org/D130506>.
>
> @ckissane What I want to demonstrate in this patch is that we should use multiple patches adding zstd support to different components, instead of having a monolithic D130516 <https://reviews.llvm.org/D130516>.
>
> - https://gregoryszorc.com/blog/2020/01/07/problems-with-pull-requests-and-how-to-fix-them/ "Previous studies have found that the number of useful comments decreases and the review latency increases as the size of the change increases."
> - When we add zstd support to various components, they should have dedicated tests.
> - Each component is cared by its maintainers/regular reviewers/users. Having a refactoring is fine, but bundling zstd features in the huge patch may surprise them. There are many factors to consider, including what to do when zstd support is unavailable.
> - Many components don't necessarily migrate to zstd.
>
> Note: Having the big change including everything is still very useful, as it gives the full picture and may enlighten useful design discussions.
>
> FWIW if I were to create all patches, I would do these. The ordering is somewhat loose, but I'll start with llvm-objcopy
>
> - add llvm-objcopy support
> - add MC support (this can be tested with llvm-objcopy)
> - add clang -gz=zstd support
> - add lld input support (D129406 <https://reviews.llvm.org/D129406>)
> - add lld output support (after MC support, I actually have a local patch)
> - add lldb support
> - ...

I totally agree!
we could try to at least get in my compression class refactor and cl::opt<CompressionAlgorithm*> support as a patch though if you think that's appropriate?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130458