[PATCH] D130516: [llvm] compression classes
David Blaikie via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 1 14:33:43 PDT 2022
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
More information about the cfe-commits
mailing list