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

balazske added inline comments. Comment at: lib/AST/ASTImporter.cpp:8197 -void ASTImporter::ImportDefinition(Decl *From) { +Error ASTImporter::ImportDefinition_New(Decl *From) { Decl *To = Import(From); a_sidorin wrote: > ImportDefinitionOrError? The intenti

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

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land. Hi Balasz, I cannot find any problems with the latest changes. I suggest you to commit the patch to make further reviews easier. Comment at: lib/AST/ASTImporter.cpp:81

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

balazske added a comment. To the reviewers: Please accept this patch formally if you do not find any problems. This is an intermediate state of the code and there is more work that is dependent on this change. Repository: rC Clang https://reviews.llvm.org/D51633 _

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

balazske added a comment. In the current version all clang tests do pass and most of the LLDB tests. Some LLDB tests do not work on my machine, the problems exist on master too. This code is still a temporary version (possible to commit now or wait until every change is made?). Later the `Import

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

balazske marked 9 inline comments as done. balazske added inline comments. Comment at: lib/AST/ASTImporter.cpp:2683 +continue; + } else if (isa(Found)) +continue; a_sidorin wrote: > Same here. I do not know exactly why this was made, it looks

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

a_sidorin added a comment. Hi Balázs, Finally, I have completed the review. Despite the fact that this patch is huge, I found only a few issues. Comment at: lib/AST/ASTImporter.cpp:4604 + if (Error Err = ImportDeclContext(D)) +// FIXME: Really ignore the error? +cons

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

balazske added inline comments. Comment at: lib/AST/ASTImporter.cpp:194 + // FIXME: This should be the final code. + //auto ToOrErr = Importer.Import(From); + //if (ToOrErr) { a_sidorin wrote: > Do I understand correctly that we have to use the upp

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

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

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

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

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

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(); +return

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

martong added a comment. This patch is huge, but we change here almost all implementation functions of `ASTNodeImporter` to return with either `Error` or with `Expected`. We could not really come up with a cohesive but smaller patch because of the recursive nature of the importer. (We are open t