[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

Wed Aug 17 10:38:02 PDT 2022

ckissane added inline comments.

================
Comment at: llvm/include/llvm/Support/Compression.h:95-98
+  static CompressionSpecRef Unknown;
+  static CompressionSpecRef None;
+  static CompressionSpecRef Zlib;
+  static CompressionSpecRef ZStd;
----------------
dblaikie wrote:
> ckissane wrote:
> > dblaikie wrote:
> > > Generally we don't want more variables that need global constructors in LLVM - so these should probably be function-local statics in functions instead.
> > > (I don't think we need a CompressionSpecRef for `Unknown` or `None`, though)
> > these are just shortcuts to the function local statics of `CompressionSpecRef getCompressionSpec(uint8_t Kind)`
> Yeah, though they're still globals with non-trivial construction, which is to be avoided in LLVM ( https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors )
> 
> So they should either be removed (I think that's probably the right tradeoff, and the one I showed in my proposal - callers that want only one specific compression algorithm can pay the small runtime overhead of going through the switch unnecessarily) or replaced with global functions that use function local statics so the initialization is only paid when the functions are called, and not by every program that links in LLVM for any reason.
I concur with removal

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131992