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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
