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