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