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

vsapsai updated this revision to Diff 470915. vsapsai added a comment. Rebase and diagnose attribute mismatch for function parameters. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135472/new/ https://reviews.llvm.org/D135472 Files: clang/includ

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

ChuanqiXu added inline comments. Comment at: clang/lib/AST/ODRHash.cpp:479-480 + + llvm::copy_if(D->attrs(), std::back_inserter(HashableAttrs), +[](const Attr *A) { return !A->isImplicit(); }); +} aaron.ballman wrote: > ChuanqiXu wrote: > > aaron

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

aaron.ballman added a comment. In D135472#3856596 , @vsapsai wrote: > 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.t

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

vsapsai added a comment. To fix pre-merge errors like error: 'error' diagnostics seen but not expected: File .../clang/test/Modules/Output/odr_hash.cpp.tmp/Inputs/first.h Line 3797: ... found 'AlignedDoublePtr' with attribute ' __attribute__((align_value(0xb7dc9b8)))' posted D135931

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

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. LGTM basically. Our discussion is mainly about the future work in Attr.td and not this patch. So I think they are not blocking issues. Comment at: clang/lib/AST/ODRDia

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

vsapsai marked an inline comment as done. vsapsai added inline comments. Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:852 +def err_module_odr_violation_attribute : Error< + "%q0 has different definitions in different modules; first difference is " + "%select{defi

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

vsapsai updated this revision to Diff 467570. vsapsai added a comment. Fix the starting point of the diff. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135472/new/ https://reviews.llvm.org/D135472 Files: clang/include/clang/AST/ODRDiagsEmitter.

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

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

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

vsapsai updated this revision to Diff 467564. vsapsai added a comment. - Small fixes to address review comments. - Try to make diagnostics more understandable. - Check attributes on typedefs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135472/new/

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

aaron.ballman added inline comments. 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: > aaron.ballman wrote: > > C

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

ChuanqiXu added inline comments. Comment at: clang/lib/AST/ODRHash.cpp:479-480 + + llvm::copy_if(D->attrs(), std::back_inserter(HashableAttrs), +[](const Attr *A) { return !A->isImplicit(); }); +} aaron.ballman wrote: > ChuanqiXu wrote: > > vsaps

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

aaron.ballman added inline comments. Comment at: clang/lib/AST/ODRDiagsEmitter.cpp:302 +llvm::raw_string_ostream OutputStream(FullText); +A->printPretty(OutputStream, Ctx.getPrintingPolicy()); +return OutputStream.str(); vsapsai wrote: > aaron.ballman

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

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 (t

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

vsapsai added inline comments. Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:852 +def err_module_odr_violation_attribute : Error< + "%q0 has different definitions in different modules; first difference is " + "%select{definition in module '%2'|defined here}1 found

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

vsapsai added a comment. In 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

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

erichkeane added a comment. Most of my concerns change based on the feedback others have given, so after those are dealt with, I'll do another depever view. Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:852 +def err_module_odr_violation_attribute : Error< + "%q0

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

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/ODRDiagsEm

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

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

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

vsapsai added inline comments. Comment at: clang/lib/AST/ODRHash.cpp:479-480 + + llvm::copy_if(D->attrs(), std::back_inserter(HashableAttrs), +[](const Attr *A) { return !A->isImplicit(); }); +} I'm not sure `isImplicit` is the best indicator of

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

vsapsai created this revision. vsapsai added reviewers: rtrieu, aaron.ballman, ChuanqiXu, Bigcheese. Herald added a subscriber: ributzka. Herald added a project: All. vsapsai requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Arguments not for