[PATCH] D47632: [ASTImporter] Refactor Decl creation

[Aleksei Sidorin via Phabricator via cfe-commits]

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