vsapsai added a comment.
In D138859#3955806 <https://reviews.llvm.org/D138859#3955806>, @ChuanqiXu wrote:
> And I think we should add tests for this. e.g., in the ASTImporterTests.cpp.
I'm not sure which test would be useful in this case. We kinda already test
that different modules agree which attributes should be ODR hashable. That is
enforced by detecting attribute mismatches in odr_hash.cpp (if attributes
aren't hashable - no mismatches).
================
Comment at: clang/include/clang/Basic/Attr.td:552-553
bit LateParsed = 0;
+ // Set to true for attributes that should participate in ODR hashing.
+ bit IsODRHashable = 0;
// Set to false to prevent an attribute from being propagated from a template
----------------
aaron.ballman wrote:
> Do we want to change the default for any of the derived classes? e.g., should
> something that is a `TypeAttr` or `DeclOrTypeAttr` default to being ODR
> hashable because they impact the type system?
>
> Also, can we expand the comment somewhat to help folks understand the
> circumstances under which they should set that to 1?
At this point I don't know what the default value should be, for that I'll need
to see how many attributes should be hashable. This change is mostly to prove
the direction is viable and reasonable. And TypeAttr, DeclOrTypeAttr is a good
starting point to decide which attributes should be hashed.
Good idea with expanding the comment. Though I need some time to prepare
non-abhorrent explanation (gosh, docs are harder than code).
================
Comment at: clang/lib/AST/AttrImpl.cpp:16
#include "clang/AST/Expr.h"
+#include "clang/AST/ODRHash.h"
#include "clang/AST/Type.h"
----------------
ChuanqiXu wrote:
> Is this change necessary?
Yep. AttrImpl.cpp includes "clang/AST/AttrImpl.inc" where, for example, we have
```lang=c++
void AlignValueAttr::processODRHash(
llvm::FoldingSetNodeID &ID, ODRHash &Hash) const {
ID.AddInteger(Attr::getKind());
const Expr *AlignmentExpr = getAlignment();
ID.AddBoolean(AlignmentExpr);
if (AlignmentExpr)
Hash.AddStmt(AlignmentExpr);
}
```
and we need the full declaration of `ODRHash` to call `ODRHash::AddStmt`.
================
Comment at: clang/lib/AST/AttrImpl.cpp:242
#include "clang/AST/AttrImpl.inc"
----------------
AttrImpl.inc include I've mentioned.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138859/new/
https://reviews.llvm.org/D138859
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
