bruno added a comment.
> - 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.
The reason is that in C++ ODR diagnostics for TagDecl are handled as part of
CXXRecordDecl, I didn't want add a path in TagDecl for C/ObjC only.
> - Are anonymous structs working? Worth adding test cases.
They are at the top level but not nested ones, I fixed that and should upload a
new patch with that next.
> - 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.
Done in upcoming patch.
> - 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.
Ditto.
> Heads up in case it affects you refactoring work:
> While looking through this code, I found a bug I previously made. I fixed
> it with a small change, but that lies in the middle of your refactoring
> during FieldDecl handling. The change is here:
>
> https://reviews.llvm.org/rGa60e8927297005898b10a46300d929ba5cf7833c
Thanks for the heads up @rtrieu, I end up including that as part of
rG90f58eaeff5f
<https://reviews.llvm.org/rG90f58eaeff5f1d5017e7b689fac79180cdfa0160>
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
