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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

(still a fair few unhandled comments from the last round - guess work to address those is still ongoing)



================
Comment at: llvm/include/llvm/Support/Compression.h:95-98
+  static CompressionSpecRef Unknown;
+  static CompressionSpecRef None;
+  static CompressionSpecRef Zlib;
+  static CompressionSpecRef ZStd;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131992