[PATCH] D130516: [llvm] compression classes

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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) {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130516