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