[PATCH] D47632: [ASTImporter] Refactor Decl creation

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: lib/AST/ASTImporter.cpp:161
+      ToD->IdentifierNamespace = FromD->IdentifierNamespace;
+      if (FromD->hasAttrs())
+        for (const Attr *FromAttr : FromD->getAttrs())
----------------
a_sidorin wrote:
> Should we move the below operations into `updateAttrAndFlags()` and use it instead?
`updateAttrAndFlags` should handle only those properties of a `Decl` which can change during a subsequent import. Such as `isUsed` and `isReferenced`.

There are other properties which cannot change, e.g. `isImplicit`.
Similarly, `Decl::hasAttrs()` refers to the syntactic attributes of a declaration, e.g. `[[noreturn]]`, which will not change during a subsequent import.

Perhaps the `Attr` syllable is confusing in the `updateAttrAndFlags()` function. Thus I suggest a new name: `updateFlags()`.


================
Comment at: lib/AST/ASTImporter.cpp:1587
   StructuralEquivalenceContext Ctx(
       Importer.getFromContext(), Importer.getToContext(),
+      Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer),
----------------
a_sidorin wrote:
> (Thinking out loud) We need to introduce some method to return `StructuralEquivalenceContext` in ASTImporter. But this is an item for a separate patch, not this one.
Yes, I agree. Or perhaps we should have a common `isStructuralMatch(Decl*, Decl*)` function which is called by the other overloads of `isStructuralMatch`.


================
Comment at: lib/AST/ASTImporter.cpp:1890
   TypedefNameDecl *ToTypedef;
-  if (IsAlias)
-    ToTypedef = TypeAliasDecl::Create(Importer.getToContext(), DC, StartL, Loc,
-                                      Name.getAsIdentifierInfo(), TInfo);
-  else
-    ToTypedef = TypedefDecl::Create(Importer.getToContext(), DC,
-                                    StartL, Loc,
-                                    Name.getAsIdentifierInfo(),
-                                    TInfo);
+  if (IsAlias && GetImportedOrCreateDecl<TypeAliasDecl>(
+                     ToTypedef, D, Importer.getToContext(), DC, StartL, Loc,
----------------
a_sidorin wrote:
> This is not strictly equivalent to the source condition. If GetImportedOrCreateDecl returns false, we'll fall to the `else` branch, and it doesn't seem correct to me.
Thanks, this is a very good catch. Fixed it.


================
Comment at: lib/AST/ASTImporter.cpp:6905
     Decl *ToD = Pos->second;
+    // FIXME: remove this call from this function
     ASTNodeImporter(*this).ImportDefinitionIfNeeded(FromD, ToD);
----------------
balazske wrote:
> I think this comment is not needed (or with other text). There is a case when `GetAlreadyImportedOrNull` is called during import of a Decl that is already imported but its definition is not yet completely imported. If this call is here we have after `GetAlreadyImportedOrNull` a Decl with complete definition. (Name of the function is still bad: It does not only "get" but makes update too. The `ImportDefinitionIfNeeded` call can be moved into the decl create function?)
Yes, I agree. Changed the text of the code, because I believe that we should do the import of the definition on the call side of `GetAlreadyImportedOrNull` for the case where it is needed (in `ImportDeclParts`).


Repository:
  rC Clang

https://reviews.llvm.org/D47632