[PATCH] D135472: [ODRHash] Hash attributes on declarations.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

I guess the reason why we didn't check odr violation for attributes is that the attributes was not a part of declarations in C++ before. But it is odd to skip the check for attributes from the perspective of users.  So I think this one should be good. The only concern is that it misses too many checks for attributes. Do you plan to add it in near future or in longer future? And I guess we need to mention this in ReleaseNotes in `Potential Breaking Changes` section.



================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:337-354
+    if (!LHS || !RHS || LHS->getKind() != RHS->getKind()) {
+      DiagError(AttributeKind)
+          << (LHS != nullptr) << LHS << FirstDecl->getSourceRange();
+      DiagNoteAttrLoc(LHS);
+      DiagNote(AttributeKind)
+          << (RHS != nullptr) << RHS << SecondDecl->getSourceRange();
+      DiagNoteAttrLoc(RHS);
----------------
I feel like we can merge these two statements.


================
Comment at: clang/lib/AST/ODRHash.cpp:479-480
+
+  llvm::copy_if(D->attrs(), std::back_inserter(HashableAttrs),
+                [](const Attr *A) { return !A->isImplicit(); });
+}
----------------
vsapsai wrote:
> I'm not sure `isImplicit` is the best indicator of attributes to check, so suggestions in this area are welcome. I think we can start strict and relax some of the checks if needed.
> 
> If people have strong opinions that some attributes shouldn't be ignored, we can add them to the tests to avoid regressions. Personally, I believe that alignment and packed attributes should never be silently ignored.
Agreed. I feel `isImplicit` is enough for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135472