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) {
----------------
ckissane wrote:
> 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.
though it will make a lot of existing code patterns less clear, and more verbose
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
