ckissane added inline comments.
================
Comment at: llvm/lib/Support/Compression.cpp:30-32
+ZStdCompressionAlgorithm
+ *llvm::compression::ZStdCompressionAlgorithm::Instance =
+ new ZStdCompressionAlgorithm();
----------------
dblaikie wrote:
> leonardchan wrote:
> > Perhaps for each of these, you could instead have something like:
> >
> > ```
> > ZStdCompressionAlgorithm *getZStdCompressionAlgorithm() {
> > static ZStdCompressionAlgorithm* instance = new ZStdCompressionAlgorithm;
> > return instance;
> > }
> > ```
> >
> > This way the instances are only new'd when they're actually used.
> Yep, I'd mentioned/suggested that (so, seconding here) elsewhere encouraging
> these to be singletons: https://reviews.llvm.org/D130516#3683384
>
> And they don't even need to be 'new'd in that case, this would be fine:
> ```
> ZstdCompressionAlgorithm &getZstdCompressionAlgorithm() {
> static ZstdCompressionAlgorithm C;
> return C;
> }
> ```
>
> Though I think maybe we don't need individual access to the algorithms, and
> it'd be fine to have only a single entry point like this:
> ```
> CompressionAlgorithm *getCompressionAlgorithm(DebugCompressionType T) {
> switch (T) {
> case DebugCompressionType::ZStd: {
> static zstd::CompressionAlgorithm Zstd;
> if (zstd::isAvailable())
> return &Zstd;
> }
> ...
> }
> return nullptr;
> }
> ```
> (or, possibly, we want to return non-null even if it isn't available, if we
> include other things (like the configure macro name - so callers can use that
> name to print helpful error messages - but then they have to explicitly check
> if the algorithm is available after the call))
they currently already have singleton behavior i.e.
`llvm::compression::ZStdCompressionAlgorithm::Instance` is the only place `new
ZStdCompressionAlgorithm()` can be put into because the constructor is
protected.
I'd rather not achieve "This way the instances are only new'd when they're
actually used."
Because the rewards of that are relatively small, but it will make the code
more verbose, I think the current pattern allows the best of both worlds of the
namespace approach:
(`llvm::compression::zlib::compress` becomes
`llvm::compression::ZlibCompression->compress`)
but they can be passed as class instances.
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
