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 

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`).

  rC Clang

cfe-commits mailing list

Reply via email to