[PATCH] D21675: New ODR checker for modules

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

Thanks! I assume there's still no measurable performance impact?



================
Comment at: include/clang/AST/ODRHash.h:54
+  // in Pointers.
+  size_type NextFreeIndex;
+
----------------
Is this always the same as `Pointers.size()`?


================
Comment at: lib/AST/ODRHash.cpp:35
+  for (auto base : Record->bases()) {
+    ID.AddInteger(base.isVirtual());
+    AddQualType(base.getType());
----------------
AddBoolean for clarity maybe?


================
Comment at: lib/AST/ODRHash.cpp:335-337
+      llvm::SmallVector<const Decl *, 16> Decls(DC->decls_begin(),
+                                                DC->decls_end());
+      ID.AddInteger(Decls.size());
----------------
You will presumably need to filter out the implicit declarations before you emit the size of the list, otherwise a `DeclContext` with an implicit declaration will hash differently from one without.


================
Comment at: lib/AST/ODRHash.cpp:493-495
+    ID.AddBoolean(hasDefaultArgument);
+    if (hasDefaultArgument)
+      AddStmt(D->getDefaultArgument());
----------------
Given that `AddStmt` already writes out an "is null" flag, could you use `AddStmt(hasDefaultArgument ? D->getDefaultArgument() : nullptr)` here?


================
Comment at: lib/Serialization/ASTReader.cpp:9015-9027
+        if (FirstEnum->getName() != SecondEnum->getName()) {
+          Diag(FirstEnum->getLocStart(),
+               diag::err_module_odr_violation_mismatch_decl_diff)
+              << Merge.first << FirstModule.empty()
+              << FirstEnum->getSourceRange() << FirstModule << EnumName
+              << FirstEnum;
+          Diag(SecondEnum->getLocStart(),
----------------
Can you factor this out into a local lambda or helper class? These dozen lines are repeated quite a lot of times here with only small variations.


https://reviews.llvm.org/D21675