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