aaron.ballman added a reviewer: erichkeane.
aaron.ballman added inline comments.
================
Comment at: clang/include/clang/AST/ODRDiagsEmitter.h:119
+ /// Diagnose ODR mismatch in attributes between 2 Decl.
+ ///
----------------
================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:159
bool ODRDiagsEmitter::diagnoseSubMismatchTypedef(
const NamedDecl *FirstRecord, StringRef FirstModule, StringRef
SecondModule,
----------------
Typedefs can have attributes, so should this be updated as well? (alignment,
ext vector types, mode attributes, etc all come to mind)
================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:302
+ llvm::raw_string_ostream OutputStream(FullText);
+ A->printPretty(OutputStream, Ctx.getPrintingPolicy());
+ return OutputStream.str();
----------------
Do we want to have more control over the printing policy here? e.g., do we
really want to claim an ODR difference if one module's printing policy
specifies indentation of 8 and another specifies indentation of 4? Or if one
module prints `restrict` while another prints `__restrict`, etc?
================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:311
+ << FirstModule.empty() << FirstModule << (FirstContainer != nullptr)
+ << FirstDecl->getIdentifier() << DiffType;
+ };
----------------
You can probably drop the `getIdentifier()` here as the diagnostics engine
knows how to print named declarations properly already.
================
Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:318
+ << SecondModule << (FirstContainer != nullptr)
+ << SecondDecl->getIdentifier() << DiffType;
+ };
----------------
Same here.
================
Comment at: clang/lib/AST/ODRHash.cpp:479-480
+
+ llvm::copy_if(D->attrs(), std::back_inserter(HashableAttrs),
+ [](const Attr *A) { return !A->isImplicit(); });
+}
----------------
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.
================
Comment at: clang/lib/AST/ODRHash.cpp:494
+
+ // FIXME: This should be auto-generated as part of Attr.td
+ switch (A->getKind()) {
----------------
100% agreed.
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
