[PATCH] D130516: [Support] compression classes

[David Blaikie via Phabricator via cfe-commits]

dblaikie 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();
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130516