[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

(I was out of town last week so I did not respond then.)

In D131992#3748306 <https://reviews.llvm.org/D131992#3748306>, @dblaikie wrote:

> @MaskRay I think this change is probably the best point of comparison/demonstration of this patch direction (taking some minor liberties with this patch to simplify things somewhat in ways that have already been discussed/seem quite reasonable - specifically having `getCompresisonSpec` return a reference and the enum having either no "unknown" value, or `getCompressionSpec` asserting on the unknown value):

I am sorry to disagree... As previously mentioned this is an expression problem.
When we maintain no state in the compression algorithms, the OO style has more boilerplate in llvm/lib/Support and in call sites, so I prefer the FP style (free function style) instead.

(Adding a compression algorithm has a significant barrier. I think we tend to add functions more than adding types, so FP style wins here.)

>   // removing the hardcoded zlib compression kind parameter in favor of what the code would look like in the near future once it's parameterized on a compression kind
>   CompressionImplementation *Compressor = getCompressionSpec(CompressionAlgorithmKind)->Implementation;
>   bool DoCompression = Compress && DoInstrProfNameCompression && Compressor;
>   if (DoCompression) {
>     Compressor->compress(arrayRefFromStringRef(FilenamesStr),
>                                     CompressedStr,
>                                     Compressor->BestSizeLevel);
>   }
>
> I think a reasonable speculation about what the alternative would look like in the a non-OO design would be something like:
>
>   bool DoCompression = Compress && DoInstrProfNameCompression && isAvailable(CompressionAlgorithmKind);
>   if (DoCompression) {
>     compress(CompressionAlgorithmKind, arrayRefFromStringRef(FilenamesStr), CompressedStr, getBestSizeLevel(CompressionAlgorithmKind));

Most call sites do not specify the compression level, so `getBestSizeLevel(CompressionAlgorithmKind)` is omitted.

> And having these three correlated calls (`isAvailable`, `compress`, and `getBestSizeLevel`) all made independently/have to have the same compression algorithm kind passed to them independently seems more error prone & redundant (not in an actual execution/runtime efficiency perspective - I don't think any of these different designs would have materially different runtime performance - but in terms of "duplicate work, and work that could diverge/become inconsistent" - essentially the duplicate work/repeated switch statements, one in each `compression::*` generic entry point seems like a code smell to me that points towards a design that doesn't have that duplication) rather than tying these calls together and making the lookup happen once and then using the features after that. Also exposing the compression algorithm along with the availability in one operation (not exposing compress/decompress/size APIs when the algorithm isn't available) seems to have similar benefits.

In the FP style (free function style), `CompressionAlgorithmKind` is passed in multiple places. It is as if, in the OO style, the `Compressor` object is passed in multiple places.

`CompressionAlgorithmKind` is an scoped enum and there is hardly another variable of the same type in the scope of `CompressionAlgorithmKind`. So I do not see how the FP style is more error-prone.
To me, not checking `isAvailable(CompressionAlgorithmKind)` is similar to not checking the nullness of `Compressor`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131992/new/

https://reviews.llvm.org/D131992