[PATCH] D130516: [llvm] compression classes

[Cole Kissane via Phabricator via llvm-commits]

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