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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits