vsapsai added a comment.
Think I'll need to make another review pass but here are my comments so far:
- Why are you adding ODR hash support for `RecordDecl` and not `TagDecl`? We
already have support for `EnumDecl`, so `TagDecl` seems like a good candidate
to cover both. Honestly, I don't know if it is possible or a good idea but it
looks plausible enough to consider.
- Are anonymous structs working? Worth adding test cases.
- Are unions working? Didn't notice any code specifically for them but
`RecordDecl` covers both structs and unions, so they should be working and we
need to test that.
- Few testing additions. These cases might be already covered or might be low
value, so take these suggestions with a grain of salt:
- test self-referential structs like `struct Node { struct Node *next; };`
- test comparing structs and forward declarations, e.g., `struct S;` and
`struct S { ... };`, and another couple `struct S { ... };` and `struct S;
struct S { ... };` The motivation is to make sure we aren't stumped when we
cannot find struct definition or when the definition is in unexpected place.
================
Comment at: clang/lib/AST/Decl.cpp:4519
+ Hash.AddRecordDecl(this);
+ setHasODRHash();
+ ODRHash = Hash.CalculateHash();
----------------
To be consistent with the existing code, it is better to call it as
`setHasODRHash(true)`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71734/new/
https://reviews.llvm.org/D71734
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
