ckissane added a comment.
In D130516#3694422 <https://reviews.llvm.org/D130516#3694422>, @dblaikie wrote:
> The current code here still seems more complicated than I'd prefer - looks
> like currently the size/speed/default levels are currently unused, so maybe
> we can omit those for now, knowing they will be added?
> And the CompressionKind with all its operator overloads seems like a lot of
> surface area that is pretty non-obvious for usage - boolean testable, logical
> operator overloads, etc.
> Could we have only one decompress/compress function each, for now?
> & maybe leave out the name/enum from the base class for now, add it in later
> (& I think I mentionted in another comment those properties can be
> non-virtual, maybe even direct const members - passed into the base through
> the ctor from the derived class)
Yes, I can continue to trim down the implementation! I agree with your
sentiment.
> Maybe it's easier if I either post a patch, or at least more explicitly flesh
> out what I'm picturing/proposing/suggesting:
> Header:
>
> struct CompressionAlgorithm {
> virtual void Compress(...);
> virtual void Decompress(...);
> };
> enum class CompressionType {
> Zlib, Zstd
> };
> CompressionAlgorithm *getCompressionAlgorithm(CompressionType);
>
> Implementation:
>
> #if LLVM_ENABLE_ZLIB
> struct ZlibCompressionAlgorthim : CompressionAlgorithm {
> void Compress(...) { ... }
> void Decompress(...) { ...}
> }
> #endif
> ...
> CompressionAlgorithm *getCompressionAlgorithm(CompressionType T) {
> switch (T) {
> case CompressionType::Zlib: {
> #if LLVM_ENABLE_ZLIB
> static ZlibCompressionAlgorithm A;
> return &A;
> #else
> break;
> #endif
> }
> ...
> }
> return nullptr;
> }
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.
> Usage:
>
> if (CompressionAlgorithm *C = getCompressionAlgorithm(CompressionType::Zlib) {
>
> C->compress(...);
>
> }
>
>
currently, you can do
if (CompressionKind C = CompressionKind::Zlib) {
C->compress(...);
}
> And, yeah, I think it'd be suitable to eventually add name, type,
> size/speed/default levels:
>
> struct CompressionAlgorithm {
> const StringRef Name;
> const CompressionType Type;
> const int DefaultLevel;
> const int BestSizeLevel;
> const int BestSpeedLevel;
> virtual void Compress(...);
> virtual void Decompress(...);
> protected:
> CompressionAlgorithm(StringRef Name, CompressionType Type, ...) :
> Name(Name), Type(Type), ... {}
> }
>
> struct ZlibCompressionAlgorithm : CompressionAlgorithm {
> ZlibCompressionAlgorithm() : CompressionAlgorithm("zlib",
> CompressionType::Zlib, 5, 10, 1) { }
> /* as before */
> };
> ...
>
> Though those can be added as needed - good to keep in mind that they're a
> useful direction to go, but might simplify the review/discussion to omit them
> for now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130516/new/
https://reviews.llvm.org/D130516
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
