[PATCH] D130516: [llvm] compression classes

[Cole Kissane via Phabricator via cfe-commits]

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