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