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