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

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

Hi,
Did you mean to split the review only or the patches?
It is not trivial how to make incremental patches for this change, every part is connected to the other. The import functions for a type (`Decl` or `Stmt`) call imports of other types (`Expr` for example) too. If the import is changed for a single type (only `Stmt`) to the new way every other visit (for `Decl` and others) needs to be updated. And then again with the next type (`Stmt` for example). The `importSeq` function calls work with any type and the visit functions that use `importSeq` can not work if a part of the imports is the old way, another the new.
A solution can be to have the new and old import interface together for every type (with different names), then a part of the visit functions can be updated. The first change would be to introduce the new interface (that returns `Expected`) for every type, but do not change the visit functions (but a new version of every other "helper" function like `importDefinition` is needed). If a new version of such a function is introduced and the old remains, we do not have a diff view (with relative changes) for it.



================
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,
----------------
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`.)



Repository:
  rC Clang

https://reviews.llvm.org/D51633