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:
> 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...
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
