[PATCH] D53697: [ASTImporter][Structural Eq] Check for isBeingDefined

[Gabor Marton via Phabricator via cfe-commits]

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