[PATCH] D130516: [llvm] compression classes

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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)

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;
  }

Usage:

  if (CompressionAlgorithm *C = getCompressionAlgorithm(CompressionType::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