[PATCH] D71734: [ODRHash] Hash `RecordDecl` and diagnose discovered mismatches.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/lib/AST/Decl.cpp:4881-4882
+  // For RecordDecl the ODRHash is stored in the remaining 26
+  // bit of RecordDeclBits, adjust the hash to accomodate.
+  setODRHash(Hash.CalculateHash() >> 6);
+  return RecordDeclBits.ODRHash;
----------------
I am curious for the operation. It looks dangerous. How can we be sure that the hash value is still meaningful after remove its lower 6 bits?


================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:1550-1551
 
+bool ODRDiagsEmitter::diagnoseMismatch(const RecordDecl *FirstRecord,
+                                       const RecordDecl *SecondRecord) const {
+  if (FirstRecord == SecondRecord)
----------------
The implementation of this function looks redundant with other overloads. Of course this is not the problem of the patch. I think we can add a FIXME at least.


================
Comment at: clang/lib/AST/ODRHash.cpp:592-593
 
+void ODRHash::AddRecordDecl(const RecordDecl *Record) {
+  AddDecl(Record);
+
----------------
For the current implementation, if it makes sense to add an assertion `!isa<CXXRecordDecl>(Record);`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71734