[PATCH] D137885: [modules] Support zstd in .pcm file

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:1457
+      const llvm::compression::Format F =
+          Blob.size() >= 2 && memcmp(Blob.data(), "\x1f\x8b", 2) == 0
+              ? llvm::compression::Format::Zlib
----------------
MaskRay wrote:
> dblaikie wrote:
> > tschuett wrote:
> > > Could you put the magic number into a named constant?
> > & perhaps we should have/test a magic number for zstd too, so it's clear it's one or the other and not something else added in the future (since this isn't reving the bitcode version or anything - reading zstd compressed data with the library before this version will I guess/hopefully appear corrupted, but we could avoid that being the failure mode in the future when another compression scheme is added by checking explicitly for zstd/zlib now and reporting unknown compression scheme otherwise?)?
> > Could you put the magic number into a named constant?
> 
> I think a constant doesn't improve readability of this used-once value with a comment.
> 
> > & perhaps we should have/test a magic number for zstd too, so it's clear it's one or the other and not something else added in the future
> 
> Since the version number keeps increasing, when a new algorithm is added, they will see a version mismatch error...
> > & perhaps we should have/test a magic number for zstd too, so it's clear it's one or the other and not something else added in the future
> 
> Since the version number keeps increasing, when a new algorithm is added, they will see a version mismatch error...

Oh, right, sorry, got muddled - thinking bitcode & then thinking LLVM IR bitcode guarantees, super stable, but the AST's super unstable/closely version locked anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137885