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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
