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

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,

To make the review proces faster, you can split the review on separate parts for Stmts, Decls, Types, etc. Otherwise, the review can last for too long.



================
Comment at: lib/AST/ASTImporter.cpp:162
+        return llvm::make_error<ImportError>();
+    return llvm::Error::success();
+  }
----------------
Can we get rid from namespace specifier usages on Error and None? As I see, even in this patch Error is used without it sometimes.


================
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,
----------------
The changes for argument indentation look redundant. Is it a result of clang-format?


================
Comment at: lib/AST/ASTImporter.cpp:840
+template <>
+Expected<TemplateArgumentLoc>
+ASTNodeImporter::import(const TemplateArgumentLoc &TALoc) {
----------------
Could you please add some newlines to this function? Its control flow is not trivial so some blocks need to be separated.


Repository:
  rC Clang

https://reviews.llvm.org/D51633