[PATCH] D51633: [ASTImporter] Added error handling for AST import.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.
Herald added a subscriber: Szelethus.

Hi Balasz,
It's going to take some time to review the whole patch.



================
Comment at: lib/AST/ASTImporter.cpp:194
+      // FIXME: This should be the final code.
+      //auto ToOrErr = Importer.Import(From);
+      //if (ToOrErr) {
----------------
Do I understand correctly that we have to use the upper variant because ASTImporter API is unchanged?


================
Comment at: lib/AST/ASTImporter.cpp:417
 
-    bool ImportDefinition(RecordDecl *From, RecordDecl *To,
-                          ImportDefinitionKind Kind = IDK_Default);
-    bool ImportDefinition(VarDecl *From, VarDecl *To,
-                          ImportDefinitionKind Kind = IDK_Default);
-    bool ImportDefinition(EnumDecl *From, EnumDecl *To,
-                          ImportDefinitionKind Kind = IDK_Default);
-    bool ImportDefinition(ObjCInterfaceDecl *From, ObjCInterfaceDecl *To,
-                          ImportDefinitionKind Kind = IDK_Default);
-    bool ImportDefinition(ObjCProtocolDecl *From, ObjCProtocolDecl *To,
-                          ImportDefinitionKind Kind = IDK_Default);
-    TemplateParameterList *ImportTemplateParameterList(
+    Error ImportDefinition(
+        RecordDecl *From, RecordDecl *To,
----------------
balazske wrote:
> a_sidorin wrote:
> > The changes for argument indentation look redundant. Is it a result of clang-format?
> clang-format aligns start of new argument lines to the `(` character above. Indentations are not always consistent in this file, I can reformat the whole file with clang-format.
> (But I do not like the formatting like
> ```
>   Error ImportDeclParts(NamedDecl *D, DeclContext *&DC, DeclContext *&LexicalDC,
>                         DeclarationName &Name, NamedDecl *&ToD,
>                         SourceLocation &Loc);
> ```
> if there is a `LongNamedClass::LongReturnType LongNamedClass::LongNamedFunction`.)
> 
No need to reformat the whole file. As I see, the indentation changes are not breaking and too wide to roll them back so we can leave them as-is.


================
Comment at: lib/AST/ASTImporter.cpp:1087
 
-  QualType ClassType = Importer.Import(QualType(T->getClass(), 0));
-  return Importer.getToContext().getMemberPointerType(ToPointeeType,
-                                                      ClassType.getTypePtr());
+  ExpectedType ClassTypeOrErr = import(QualType(T->getClass(), 0));
+  if (!ClassTypeOrErr)
----------------
Nice catch!


================
Comment at: lib/AST/ASTImporter.cpp:1658
   }
+  llvm::SmallVector<Decl*, 8> ImportedDecls;
+  for (auto *From : FromDC->decls()) {
----------------
Space before '*' is needed.


================
Comment at: lib/AST/ASTImporter.cpp:1663
+      // Ignore the error, continue with next Decl.
+      consumeError(ImportedOrErr.takeError());
+  }
----------------
Silent fail doesn't look correct in case of structures. Should we add a FIXME?


================
Comment at: lib/AST/ASTImporter.cpp:1953
 
-  return false;
+Expected<TemplateArgument>
+ASTNodeImporter::ImportTemplateArgument(const TemplateArgument &From) {
----------------
Should we add a FIXME for removing this method?


================
Comment at: lib/AST/ASTImporter.cpp:2266
+      ToD, D, Importer.getToContext(), DC, ToNamespaceLoc, ToAliasLoc,
+      ToIdentifier,ToQualifierLoc, ToTargetNameLoc, ToNamespace))
     return ToD;
----------------
Space after comma is lost.


================
Comment at: lib/AST/ASTImporter.cpp:2656
           }
+          if (IsFriendTemplate)
+            continue;
----------------
This move looks like an additional functional change. Could you please explain the reason?


================
Comment at: lib/AST/ASTImporter.cpp:2683
+        continue;
+      } else if (isa<ValueDecl>(Found))
+        continue;
----------------
Same here.


================
Comment at: lib/AST/ASTImporter.cpp:2706
     CXXRecordDecl *D2CXX = nullptr;
-    if (auto *DCXX = dyn_cast<CXXRecordDecl>(D)) {
+    if (auto *DCXX = llvm::dyn_cast<CXXRecordDecl>(D)) {
       if (DCXX->isLambda()) {
----------------
We usually don't qualify casts.


================
Comment at: lib/AST/ASTImporter.cpp:4136
   return ToProto;
 }
 
----------------
// Stopped here.


Repository:
  rC Clang

https://reviews.llvm.org/D51633