[PATCH] D130516: [Support] compression classes

[Cole Kissane via Phabricator via llvm-commits]

ckissane added inline comments.


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:2003-2004
   // consumers will not want its contents.
+  llvm::compression::CompressionAlgorithm CompressionScheme =
+      llvm::compression::ZlibCompressionAlgorithm();
+
----------------
dblaikie wrote:
> ckissane wrote:
> > dblaikie wrote:
> > > Doesn't this cause slicing & end up with the base implementation?
> > > 
> > > (also the base class `CompressionAlgorithm` has no virtual functions, so I'm not sure how this is meant to work - does this code all work? Then I must be missing some things - how does this work?)
> > You are correct to observe that this patch does not fully pass around pointers to instances of the classes, however, because I don't pass pointers and the currently repetitive nature of the compression classes, this still functions correctly.
> > In short, a follow-up patch (which I will shortly upload) will convert this to using class instances and passing those around.
> > Including reworking functions throughout llvm-project to take advantage of this.
> > I am aiming to take this 2 step process to cut down on making an already large pass larger.
> > Let me know if you have any concerns or ideas.
> But I'm not sure how this patch works correctly - wouldn't the line below (`CompressionScheme.supported()`) call `CompressionAlgorithm::supported()` which always returns false?
good catch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130516