[PATCH] D130516: [llvm] compression classes

[Cole Kissane via Phabricator via llvm-commits]

ckissane added inline comments.


================
Comment at: llvm/lib/ProfileData/Coverage/CoverageMappingWriter.cpp:54-60
+  OptionalCompressionScheme = compression::noneIfUnsupported(
+      (Compress && DoInstrProfNameCompression) ? OptionalCompressionScheme
+                                               : llvm::NoneType());
+
+  bool doCompression = bool(OptionalCompressionScheme);
+
+  if (doCompression) {
----------------
dblaikie wrote:
> This still seems like a lot of hoops to jump through - why "noneIfUnsupported" rather than either having the compression scheme (I think it could be the CompressionAlgorithm itself, rather than having the separate OptionalCompressionKind abstraction) either be null itself, or expose an "isAvailable" operation directly on the CompressionAlgorithm?
> 
> Even if the CompressionKind/OptionalCompressionKind/CompressionAlgorithm abstractions are kept, I'm not sure why the above code is preferred over, say:
> 
> ```
> if (Compress && DoInstrProfNameCompression && OptionalCompressionScheme /* .isAvailable(), if we want to be more explicit */) {
>   ...
> }
> ```
> 
> What's the benefit that `noneIfUnsupported` is providing? (& generally I'd expect the `Compress && DoInstrProfNameCompression` to be tested/exit early before even naming/constructing/querying/doing anything with the compression scheme/algorithm/etc - so there'd be no need to combine the tests for availability and the tests for whether compression was requested)
> 
> Perhaps this API is motivated by a desire to implement something much closer to the original code than is necessary/suitable? Or some other use case/benefit I'm not quite understanding yet?
I shall remove `noneIfUnsupported`. You express good points, we can simply check `if(OptionalCompressionScheme && *OptionalCompressionScheme)` where necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130516