martong marked 2 inline comments as done. martong added a comment. > I wonder if it is possible to get into situation where non-equivalent decls > are marked equivalent with this patch? If yes, we can create a mapping > between decls being imported and original decls as an alternative solution. > However, I cannot find any counterexample.
I don't think so. This change is the natural extension what we already do (in line 1021 and 1022) with `getDefinition()`. `getDefinition()` works just as we would expect with a simple `RecordDecl`: `getDefinition()` returns a non-nullptr if `isBeingDefined()` is true. However, `getDefinition()` may return a non-nullptr if `D` is a `CXXRecordDecl` even if `D` is being defined. CXXRecordDecl *getDefinition() const { // We only need an update if we don't already know which // declaration is the definition. auto *DD = DefinitionData ? DefinitionData : dataPtr(); return DD ? DD->Definition : nullptr; } This all depends on whether `DefinitionData` is set. And we do set that during `ImportDefinition(RecordDecl *,)`. And then we start importing methods and fields of the `CXXRecordDecl` via `ImportDeclContext` before the call to `completeDefinition()` which sets `IsBeingDefined`. During those imports, the `getDefinition()` of a `CXXRecordDecl` will return with a non-nullptr value and we would go on checking the fields, but we are in the middle of importing the fields (or methods). ================ Comment at: lib/AST/ASTStructuralEquivalence.cpp:1037 + // equality and we assume that the decls are equal. + if (D1->isBeingDefined() || D2->isBeingDefined()) + return true; ---------------- a_sidorin wrote: > Is it worth it to assert if only one Decl should be in `isBeingDefined()` > state at time? Strictly speaking `D1` will always come from the "From" context and such it should report true for `isBeingDefined`. But the structural equivalence logic should be independent from the import logic ideally, so I would not add that assert. ================ Comment at: unittests/AST/ASTImporterTest.cpp:3729 +TEST_P(ASTImporterTestBase, ImportingTypedefShouldImportTheCompleteType) { + // We already have an incomplete underlying type in the "To" context. ---------------- a_sidorin wrote: > Looks like this test is from another patch (D53693)? Yes, exactly, good catch, I just removed it. Repository: rC Clang https://reviews.llvm.org/D53697 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits