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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai marked 3 inline comments as done.
vsapsai added a comment.

Given all the discussion about which attributes should be added to ODR hash, I think it is useful at this point to have TableGen infrastructure to get this information from Attr.td. So I'll work on that.



================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:159
 
 bool ODRDiagsEmitter::diagnoseSubMismatchTypedef(
     const NamedDecl *FirstRecord, StringRef FirstModule, StringRef SecondModule,
----------------
vsapsai wrote:
> aaron.ballman wrote:
> > Typedefs can have attributes, so should this be updated as well? (alignment, ext vector types, mode attributes, etc all come to mind)
> It should be tested for sure, thanks for pointing it out.
Typedefs are done. But while adding support for them I've realized we don't check method parameters. So it requires a little bit more work.


================
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:
> vsapsai wrote:
> > 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)`
> > ?
> Sorry for the ambiguity. Since `LHS->getKind() != RHS->getKind()` is covered by `ComputeAttrODRHash(LHS) != ComputeAttrODRHash(RHS)`. I feel like it is reasonable to:
> 
> ```
> if (!LHS || !RHS || ComputeAttrODRHash(LHS) != ComputeAttrODRHash(RHS)) {
>     DiagError();
>     DiagNote();
>     DiagNote();
>     DiagNoteAttrLoc();
>     return true;
> }
> ```
There are 2 separate cases to improve the diagnostic. In the first case we'd have
```
... first difference is definition in module 'FirstModule' found attribute 'enum_extensibility'
attribute specified here
but in 'SecondModule' found no attribute
```

And if we reach the second ones, it implies the kind of attributes is the same and the only difference is attribute arguments, so the diagnostics are like
```
first difference is definition in module 'FirstModule' found attribute ' __attribute__((enum_extensibility("open")))'
attribute specified here
but in 'SecondModule' found different attribute argument ' __attribute__((enum_extensibility("closed")))'
attribute specified here
```

>From my limited experience, it is actually useful to have more details than trying to figure out the difference in attributes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135472