[PATCH] D130516: [llvm] compression classes

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

(still lots of outstanding comments from my last round, so I won't reiterate those - but waiting for some responses to them)



================
Comment at: llvm/lib/Support/Compression.cpp:30-32
+ZStdCompressionAlgorithm
+    *llvm::compression::ZStdCompressionAlgorithm::Instance =
+        new ZStdCompressionAlgorithm();
----------------
ckissane wrote:
> 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.
Global constructors are to be avoided in LLVM: https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors
(also these objects don't need to be dynamically allocated with `new` - they can be directly allocated (as static locals though, not as globals))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130516