[PATCH] D130516: [llvm] compression classes

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

> Yes, I can continue to trim down the implementation! I agree with your sentiment.

Thanks! This update helps - though I think we'll still want to further isolate the different pieces of this change/reduce this further.

> I agree with some of this, I have some strong thoughts I would like to work out about the whole nullptr being none or unsupported a little preemptively IMO.

Could you clarify what use cases you have in mind that require the nuance between none and unsupported? (arguably this accessor function could assert when passed None - would that make it simpler? Then the only null return would be unsupported)

>>   Usage:
>>
>> if (CompressionAlgorithm *C = getCompressionAlgorithm(CompressionType::Zlib) {
>>
>>   C->compress(...);
>>
>> }
>>
>>   
>
> currently, you can do
>
>   if (CompressionKind C = CompressionKind::Zlib) {
>     C->compress(...);
>   }

The implementation complexity is a concern too, though. I think having CompressionKind, boolean conversions and logical operator overloads, etc, in addition to the CompressionAlgorithm doesn't seem to provide enough to justify the complexity - but perhaps I'm missing some context/understanding of the values those features provide?

What sort of use cases do you have in mind that necessitate that complexity/functionality? (specifically I see a lot of ` || llvm::NoneType()` which seems really obtuse/unclear why a user of the API would think to do that/understand that was the right/necessary thing to do)

Maybe for comparison purposes it'd be good not to replace this API but to add it on top of the underlying API so only one callsite can be updated, rather than all the changes necessary to update all clients in one go (& in this way maybe omit some of the functionality in this first patch, since it won't have to cover all use cases - eg: those extra fields (name/compression levels (best size/speed/default), etc) could possibly be omitted from the first version of this patch, so the patch only adds enough functionality for one of the compresion clients (like MC, to match/compare with @MaskRay's patch) rather than all of them)



================
Comment at: llvm/include/llvm/Support/Compression.h:48-51
+  Error decompress(ArrayRef<uint8_t> Input, uint8_t *UncompressedBuffer,
+                   size_t &UncompressedSize) {
+    return Decompress(Input, UncompressedBuffer, UncompressedSize);
+  }
----------------
Could we skip this wrapper & just have the underlying function (& also we shouldn't be overloading by case like this anyway - please name all the decompress/compress functions with the same case/spelling)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130516