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