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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
