[PATCH] D130516: [llvm] compression classes

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:50-58
+  compression::CompressionAlgorithm *CompressionScheme =
+      new compression::ZlibCompressionAlgorithm();
+
+  CompressionScheme =
+      CompressionScheme->when(Compress && DoInstrProfNameCompression)
+          ->whenSupported();
+  bool doCompression = CompressionScheme->notNone();
----------------
This seems a bit too convoluted for me.

I'd think something like:
```
if (DoInstrProfNameCompression) {
  if (CompressionAlgorithm *C = getZlibCompressionAlgorithm())
    C->compress(...);
}
```
Or even have `getCompressionAlgorithm(SupportCompressionType::Zlib)` (like that could be the only entry point - no need for algorithm-specific accessors, that function would have one switch over `SupportCompressionType`, returning null if Unknown or Null were passed, or if the requested algorithm was not available)

I'm not sure I understand the 'when'/'whenSupported' stuff and whether there's any value/need for more details to be communicated in the not-available case other than 'false'/null/nothing (like, if it needs to communicate a reason for non-availability, that's more involved than returning null from some factory/accessor function).


================
Comment at: llvm/lib/Support/Compression.cpp:98-102
+constexpr SupportCompressionType ZlibCompressionAlgorithm::AlgorithmId;
+constexpr StringRef ZlibCompressionAlgorithm::Name;
+constexpr int ZlibCompressionAlgorithm::BestSpeedCompression;
+constexpr int ZlibCompressionAlgorithm::DefaultCompression;
+constexpr int ZlibCompressionAlgorithm::BestSizeCompression;
----------------
Maybe these don't need to be static members - if there are singleton insntances of the algorithms, they could be members of those singletons instead (possibly in the base/impl class - the derived classes could pass these values into the base ctor to initialize members in the impl or base - they could even be const public members, avoid the need for accessors (at least avoiding the need for virtual accessors, but hopefully avoiding accessors entirely))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130516