ChuanqiXu added a comment.
> 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).
It looks like you're planning to add them one by one, or do I misunderstand? It
looks not so good to add them one by one. Maybe it'll be a better idea to add
them in the Attr.td?
================
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);
----------------
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;
}
```
================
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:
> vsapsai wrote:
> > erichkeane wrote:
> > > aaron.ballman wrote:
> > > > ChuanqiXu wrote:
> > > > > 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.
> > > > The tricky part is -- sometimes certain attributes add additional
> > > > implicit attributes and those implicit attributes matter
> > > > (https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclAttr.cpp#L9380).
> > > > And some attributes seem to just do the wrong thing entirely:
> > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclAttr.cpp#L7344
> > > >
> > > > So I think `isImplicit()` is a good approximation, but I'm more
> > > > wondering what the principle is for whether an attribute should or
> > > > should not be considered part of the ODR hash. Type attributes,
> > > > attributes that impact object layout, etc all seem like they should
> > > > almost certainly be part of ODR hashing. Others are a bit more
> > > > questionable though.
> > > >
> > > > I think this is something that may need a per-attribute flag in Attr.td
> > > > so attributes can opt in or out of it because I'm not certain what ODR
> > > > issues could stem from `[[maybe_unused]]` or `[[deprecated]]`
> > > > disagreements across module boundaries.
> > > I don't think 'isImplicit' is particularly good. I think the idea of
> > > auto-adding 'type' attributes and having the 'rest' be analyzed to figure
> > > out which are important.
> > >
> > > Alternatively, I wonder if we're better off just adding ALL attributes
> > > and seeing what the fallout is. We can later decide when we don't want
> > > them to be ODR significant (which, might be OTHERWISE meaningful later!).
> > One criteria to decide which attributes should be hashed is if they affect
> > IRGen. But that's not exhaustive and I'm not sure how practical it is.
> >
> > The rule I'm trying to follow right now is if declarations with different
> > attributes can be substituted instead of each other. For example, structs
> > with different memory layout cannot be interchanged, so it is reasonable to
> > reject them.
> >
> > But maybe we should review attributes on case-by-case basis. For example,
> > for `[[deprecated]]` I think the best for developers is not to complain
> > about it but merge the attributes and then have regular diagnostic about
> > using a deprecated entity.
> One option was to hash `isa<InheritedAttr>` attributes as they are "sticky"
> and should be more significant, so shouldn't be ignored. But I don't think
> this argument is particularly convincing.
>
> At this stage I think we can still afford to add all attributes and exclude
> some as-needed because modules adoption is still limited. Though I don't have
> strong feelings about this approach, just getting more restrictive later can
> be hard.
It looks not a bad idea to add all attributes as experiments, @vsapsai how do
you feel about this?
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
