[PATCH] D53693: [ASTImporter] Typedef import brings in the complete type

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.
Herald added a reviewer: shafik.
Herald added a subscriber: gamesh411.


================
Comment at: lib/AST/ASTImporter.cpp:2310
+                D->getUnderlyingType(), FoundTypedef->getUnderlyingType())) {
+          QualType FromUT = D->getUnderlyingType();
+          QualType FoundUT = FoundTypedef->getUnderlyingType();
----------------
a_sidorin wrote:
> We can move these two vars upper and use them in the condition.
Good point, changed it.


================
Comment at: lib/AST/ASTImporter.cpp:2314
+          // already have a complete underlying type then return with that.
+          if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType())
             return Importer.MapImported(D, FoundTypedef);
----------------
a_sidorin wrote:
> This condition omits the case where both types are complete. Should we use `FromUT->isIncompleteType == FoundUT-isIncompleteType()` instead?
I think you wanted to say "both types are INcomplete".
The condition indeed omits the case where both types are incomplete and that is a potential bug, thanks for finding it!
I think the best way to handle this case is to do the something similar what is done in case of functions, i.e break the for loop once we know that the found and the to be imported Decl are structurally equivalent.
So, I added a `break` to do that and to be similar what we do in case of functions.

This is what we have with functions:
```
          if (IsStructuralMatch(D, FoundFunction)) {
            const FunctionDecl *Definition = nullptr;
            if (D->doesThisDeclarationHaveABody() &&
                FoundFunction->hasBody(Definition)) {
              return Importer.MapImported(
                  D, const_cast<FunctionDecl *>(Definition));
            }
            FoundByLookup = FoundFunction;
            break;
          }
```

I'd like to have something similar with typedefs. The goal is to have only one TypedefNameDecl with a complete type. A TypedefNameDecl with a complete type is analogous to a FunctionDecl with a definition, we want to have only one in the "To" context.
A TypedefNameDecl is also redeclarable, so on a long term we should connect a new Decl to the found previous one.




Repository:
  rC Clang

https://reviews.llvm.org/D53693