[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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