[PATCH] D130516: [llvm] compression classes

[Cole Kissane via Phabricator via llvm-commits]

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