[PATCH] D27689: Module: hash the pcm content and use it as SIGNATURE for implicit modules.

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

Generally this seems reasonable to me. I don't see any particular need to split this patch up into smaller pieces.



================
Comment at: include/clang/Serialization/ASTBitCodes.h:256
       EXTENSION_BLOCK_ID,
+      DIAGNOSTIC_OPTIONS_BLOCK_ID
     };
----------------
Add a comment describing this block.

Please also give this a name that describes its purpose (to hold records that should not be part of the signature). The signature is not a diagnostic option.


================
Comment at: include/clang/Serialization/ASTBitCodes.h:323
+      /// \brief Record code for the signature that identifiers this AST file.
+      SIGNATURE,
     };
----------------
Move this and DIAGNOSTIC_OPTIONS to a separate enum holding record types for the new block.


================
Comment at: include/clang/Serialization/ASTWriter.h:432
+  void WriteControlBlock(Preprocessor &PP, ASTContext &Context,
                              StringRef isysroot, const std::string &OutputFile);
+  ASTFileSignature WriteDiagnosticOptionBlock(ASTContext &Context);
----------------
Fix indentation


================
Comment at: include/clang/Serialization/ASTWriter.h:435
+  /// \brief Calculate hash of the pcm content and write it to SIGNATURE record.
+  ASTFileSignature WriteHash(size_t);
   void WriteInputFiles(SourceManager &SourceMgr, HeaderSearchOptions &HSOpts,
----------------
Give the parameter a name


================
Comment at: include/clang/Serialization/ASTWriter.h:501-502
+  ASTFileSignature WriteASTCore(Sema &SemaRef,
                         StringRef isysroot, const std::string &OutputFile,
                         Module *WritingModule);
 
----------------
Fix indentation


================
Comment at: include/clang/Serialization/ASTWriter.h:534-535
+  ASTFileSignature WriteAST(Sema &SemaRef, const std::string &OutputFile,
                     Module *WritingModule, StringRef isysroot,
                     bool hasErrors = false);
 
----------------
Fix indentation


================
Comment at: include/clang/Serialization/Module.h:19
 #include "clang/Basic/FileManager.h"
+#include "clang/Basic/Module.h"
 #include "clang/Basic/SourceLocation.h"
----------------
Redundant include?


================
Comment at: lib/Serialization/ASTWriter.cpp:1356
+  ASTFileSignature RetCode{{0}};
+  if (WritingModule && Context.getLangOpts().ImplicitModules)
+    RetCode = WriteHash(0);
----------------
I don't think it makes sense to use `ImplicitModules` to control this. Doing that for the old signature computation was at best a hack.

If there is no measurable performance difference from the hashing, we should just do it unconditionally in all modes. Otherwise, there should be a separate flag to control it; we should probably force that flag to be enabled when the frontend implicitly builds a module and inserts it into the module cache, but otherwise let the user control it as they see fit.


https://reviews.llvm.org/D27689