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

vsapsai updated this revision to Diff 482981. vsapsai added a comment. Herald added a project: All. Rebase and use ODRDiagsEmitter. Add more tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71734/new/ https://reviews.llvm.org/D71734 Files:

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

vsapsai planned changes to this revision. vsapsai added a comment. Need to detect mismatches in nested structs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71734/new/ https://reviews.llvm.org/D71734

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

vsapsai added a comment. Found another case that doesn't emit an error #if defined(FIRST) struct Indirect { int x; }; struct Direct { struct Indirect i; }; #elif defined(SECOND) struct Indirect { double a; }; struct Direct { struct Indirect i; }; #else

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

vsapsai added inline comments. Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:818 +// performance. +RecordDecl *Canon = static_cast(RD->getCanonicalDecl()); +if (RD == Canon || Canon->getODRHash() == RD->getODRHash()) During investigation of a

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

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

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

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

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

vsapsai added inline comments. Comment at: clang/lib/AST/DeclCXX.cpp:487 // Only calculate hash on first call of getODRHash per record. - ODRHash Hash; + class ODRHash Hash; Hash.AddCXXRecordDecl(getDefinition()); rsmith wrote: > I think this change is

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

vsapsai updated this revision to Diff 353081. vsapsai marked an inline comment as done. vsapsai added a comment. Rebase on top of "main" and address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71734/new/

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

bruno added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:9585-9587 +assert(getContext().hasSameType(FirstField->getType(), +SecondField->getType())); + rsmith wrote: > bruno wrote: > > rsmith wrote: >

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

rsmith added inline comments. Comment at: clang/lib/AST/DeclCXX.cpp:487 // Only calculate hash on first call of getODRHash per record. - ODRHash Hash; + class ODRHash Hash; Hash.AddCXXRecordDecl(getDefinition()); I think this change is no longer

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

bruno added a comment. Ping! 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

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

bruno updated this revision to Diff 244435. bruno added a comment. Address Richard's review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71734/new/ https://reviews.llvm.org/D71734 Files: clang/include/clang/AST/Decl.h

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

bruno marked 3 inline comments as done. bruno added a comment. Thanks for taking a look Richard, comments inline. Comment at: clang/include/clang/AST/Decl.h:4009-4010 + + /// Store the ODR hash for this decl. + unsigned ODRHash; }; rsmith wrote: > We

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

rsmith added inline comments. Comment at: clang/include/clang/AST/Decl.h:4009-4010 + + /// Store the ODR hash for this decl. + unsigned ODRHash; }; We shouldn't store this here; this will make all `CXXRecordDecl`s larger to store a field they will never look

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

bruno updated this revision to Diff 241855. bruno added a comment. - Update patch after Volodymyr's review. - Refactor bits done as part of rG90f58eaeff5f , update it to remove that part. Repository: rG LLVM Github

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

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

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

rtrieu added a comment. 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:

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

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

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

rtrieu added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:11007 + // Issue any pending ODR-failure diagnostics. + for (auto : RecordOdrMergeFailures) { bruno wrote: > rtrieu wrote: > > Is this just a copy of the other loop? That's a

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

bruno marked an inline comment as done. bruno added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:11007 + // Issue any pending ODR-failure diagnostics. + for (auto : RecordOdrMergeFailures) { rtrieu wrote: > Is this just a copy of the

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

rtrieu added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:11007 + // Issue any pending ODR-failure diagnostics. + for (auto : RecordOdrMergeFailures) { Is this just a copy of the other loop? That's a lot of code duplication.

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

bruno updated this revision to Diff 236965. bruno added a comment. Remove some FIXMEs that are now done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71734/new/ https://reviews.llvm.org/D71734 Files: clang/include/clang/AST/Decl.h

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

bruno updated this revision to Diff 236964. bruno added a comment. Change the approach: handle type merging for tag types using the ODRHash mechanism Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71734/new/ https://reviews.llvm.org/D71734 Files:

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

bruno marked an inline comment as done. bruno added a comment. Comment at: clang/lib/Serialization/ASTReader.cpp:9286 + false /*UseCanonicalDecls*/); + (void)Ctx.IsEquivalent(D, Canon); +} martong wrote: > Would it be possible to check the

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

martong added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:9286 + false /*UseCanonicalDecls*/); + (void)Ctx.IsEquivalent(D, Canon); +} Would it be possible to check the structural non-equivalency with `ODRHash`? I wonder

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

bruno created this revision. bruno added reviewers: rsmith, arphaman, vsapsai, martong. Herald added subscribers: cfe-commits, ributzka, dexonsmith, jkorous, rnkovacs. Herald added a reviewer: jdoerfert. Herald added a project: clang. Take `struct Z {...}` defined differently and imported from