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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D135472#3844688 <https://reviews.llvm.org/D135472#3844688>, @ChuanqiXu wrote:

> 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.

I think we haven't seen many problems with attribute mismatches because in existing code attributes are often hidden behind macros. And we have sugar `MacroQualifiedType` that causes the definitions

  #define NODEREF __attribute__((noderef))
  struct StructWithField {
    int NODEREF *i_ptr;
  };

  struct StructWithField {
    int __attribute__((noderef)) *i_ptr;
  };

to be treated as incompatible.

But the keyword `alignas` is used without macros and more attributes can be used in multiple compilers without macros. That's why I think it is useful to detect and to diagnose mismatches in attributes.

In D135472#3844688 <https://reviews.llvm.org/D135472#3844688>, @ChuanqiXu wrote:

> 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?

In the near future I was planning to add various Objective-C and Swift attributes. For other attributes I don't know which are high-value. I definitely want to check attributes affecting the memory layout (alignment, packing) and believe I've addressed them (totally could have missed something).

In D135472#3844688 <https://reviews.llvm.org/D135472#3844688>, @ChuanqiXu wrote:

> And I guess we need to mention this in ReleaseNotes in `Potential Breaking Changes` section.

Good idea, will do.



================
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);
----------------
ChuanqiXu wrote:
> I feel like we can merge these two statements.
Sorry, I don't really get what two statements you are talking about. Is it 
* `!LHS || !RHS || LHS->getKind() != RHS->getKind()`
* `ComputeAttrODRHash(LHS) != ComputeAttrODRHash(RHS)`
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135472