a_sidorin added a comment.
Hi Gabor,
I like the new syntax. There are some comments inline; most of them are just
stylish.
================
Comment at: lib/AST/ASTImporter.cpp:94
+ void updateAttrAndFlags(const Decl *From, Decl *To) {
+ // check if some flags or attrs are new in 'From' and copy into 'To'
+ // FIXME: other flags or attrs?
----------------
Comments should be complete sentences: start with a capital and end with a
period.
================
Comment at: lib/AST/ASTImporter.cpp:114
+
+ // Always use theses functions to create a Decl during import. There are
+ // certain tasks which must be done after the Decl was created, e.g. we
----------------
these?
================
Comment at: lib/AST/ASTImporter.cpp:161
+ ToD->IdentifierNamespace = FromD->IdentifierNamespace;
+ if (FromD->hasAttrs())
+ for (const Attr *FromAttr : FromD->getAttrs())
----------------
Should we move the below operations into `updateAttrAndFlags()` and use it
instead?
================
Comment at: lib/AST/ASTImporter.cpp:1587
StructuralEquivalenceContext Ctx(
Importer.getFromContext(), Importer.getToContext(),
+ Importer.getNonEquivalentDecls(), getStructuralEquivalenceKind(Importer),
----------------
(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.
================
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,
----------------
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.
================
Comment at: lib/AST/ASTImporter.cpp:4274
+ TemplateParams))
+ return ToD;
+ return ToD;
----------------
Can we just ignore the return value by casting it to void here and in similar
cases?
================
Comment at: lib/AST/ASTStructuralEquivalence.cpp:895
+ // If any of the records has external storage and we do a minimal check (or
+ // AST import) we assmue they are equivalent. (If we didn't have this
+ // assumption then `RecordDecl::LoadFieldsFromExternalStorage` could trigger
----------------
assume
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
