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:
> ckissane wrote:
> > ckissane wrote:
> > > 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
> > and sometimes you really do need to re code the exact thing
> > `noneIfUnsupported` encapsulates...
> Are there examples within LLVM that you can show compare/contrast
> `noneIfUnsupported` helps?
yes, I'll paste a couple here
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
