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:
> > 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
Ok, So I believe I was mistaken.
In older versions of this patch there was a none compression implementation
that just did a memcpy, this made a natural fall through state, which made
this type of pattern advantageous.
However, this is no longer the case.
Hence I will remove this without further adue.
Thank you for your astute observation!
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
