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