[PATCH] D130516: [llvm] compression classes

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D130516#3710903 <https://reviews.llvm.org/D130516#3710903>, @ckissane wrote:

> In D130516#3703691 <https://reviews.llvm.org/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` directly - basically the contents of `CompressionKind::operator->` could be a free/public function `const CompressionAlgorithm &getCompressionAlgorithm(enum class CompressionKind);` - and have it return a reference to the local static implementation, with a none implementation (for those profile cases where you want to pass in an algorithm if it's available, or none) and one implementation for each of zlib/zstd?
>
> I can see what you are asking for, however since its behavior is essentially the same, and still uses both enum values and class implementations, I don't see any practical advantages (though if there is something I am failing to observe let me know).

The intent is to simplify both implementation and usage because this currently feels like overkill for a fairly small abstraction benefit (compared to @maskray's posted alternative, for instance) - we're abstracting only a handful of use cases over only 2 implementations, so this shouldn't be too complicated/overengineered.

> Additionally, I can see some disadvantages in largely increasing code verbosity across the codebase. 
> A snippet of an example from elf: `if(CompressionKind::Zlib) CompressionKind::Zlib->compress...`
> would turn into `if(getCompressionAlgorithm(CompressionKind::Zlib)) getCompressionAlgorithm(CompressionKind::Zlib)->compress...`

I think in either case this should be changed to something like this (specifically avoid writing `CompressionKind::Zlib` twice):

  if (auto C = CompressionKind::Zlib)
    C->compress...



> (assuming bool operator overload is also moved to class)
> Because of this I am leaning away from such an implementation change.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130516