[PATCH] D130516: [llvm] compression classes

Cole Kissane via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Aug 5 11:12:00 PDT 2022

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

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list