[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

This revision was automatically updated to reflect the committed changes. Closed by commit rL360760: [ASTImporter] Use llvm::Expected and Error in the importer API (authored by martong, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

martong updated this revision to Diff 199569. martong added a comment. - Rebase to master Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61438/new/ https://reviews.llvm.org/D61438 Files: clang/include/clang/AST/ASTImporter.h

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

martong added a comment. Thank you guys for the review! In D61438#1501457 , @aprantl wrote: > Alright, thanks for taking the time to walk me through the thought process! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

aprantl accepted this revision. aprantl added a comment. Alright, thanks for taking the time to walk me through the thought process! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61438/new/ https://reviews.llvm.org/D61438

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

martong marked an inline comment as done. martong added inline comments. Comment at: clang/include/clang/AST/ASTImporter.h:203 /// context, or the import error. -llvm::Expected Import_New(TypeSourceInfo *FromTSI); -// FIXME: Remove this version. -TypeSourceInfo

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

aprantl added inline comments. Comment at: clang/include/clang/AST/ASTImporter.h:203 /// context, or the import error. -llvm::Expected Import_New(TypeSourceInfo *FromTSI); -// FIXME: Remove this version. -TypeSourceInfo *Import(TypeSourceInfo *FromTSI); +

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

martong added a comment. @aprantl Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61438/new/ https://reviews.llvm.org/D61438 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

martong marked an inline comment as done. martong added inline comments. Comment at: clang/include/clang/AST/ASTImporter.h:203 /// context, or the import error. -llvm::Expected Import_New(TypeSourceInfo *FromTSI); -// FIXME: Remove this version. -TypeSourceInfo

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

aprantl added inline comments. Comment at: clang/include/clang/AST/ASTImporter.h:203 /// context, or the import error. -llvm::Expected Import_New(TypeSourceInfo *FromTSI); -// FIXME: Remove this version. -TypeSourceInfo *Import(TypeSourceInfo *FromTSI); +

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

martong updated this revision to Diff 198824. martong added a comment. - Remove remaining FIXMEs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61438/new/ https://reviews.llvm.org/D61438 Files: clang/include/clang/AST/ASTImporter.h

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

martong updated this revision to Diff 198822. martong marked 3 inline comments as done. martong added a comment. - Remove FIXME and return the error - Use early return where possible and remove redundant else Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

martong marked 8 inline comments as done. martong added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:5039 + if (!ToOrErr) +// FIXME: return the error? +consumeError(ToOrErr.takeError()); aprantl wrote: > We don't

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

sgraenitz added inline comments. Comment at: lldb/source/Symbol/ClangASTImporter.cpp:68 + return *ret_or_error; +} else { + Log *log = aprantl wrote: > The `else` is redundant. Here it's necessary for the scope of `ret_or_error`. That's a bit

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

aprantl added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:5039 + if (!ToOrErr) +// FIXME: return the error? +consumeError(ToOrErr.takeError()); We don't typically commit FIXME's into LLVM code. Why not just deal

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

martong updated this revision to Diff 198623. martong added a comment. - Use LLDB_LOG_ERROR in ImportDefinitionTo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61438/new/ https://reviews.llvm.org/D61438 Files:

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

martong updated this revision to Diff 198618. martong added a comment. - Add braces to 'true' cases when 'false' case has braces - Simplify logging and error handling with LLDB_LOG_ERROR Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61438/new/

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

sgraenitz added inline comments. Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:1980 + } + return *type_or_error; } >>! In D61438#1490102, @jingham wrote: > [...] include the contents of that error n the log message? e.g: ``` if

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

a_sidorin accepted this revision. a_sidorin added a comment. This revision is now accepted and ready to land.  Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:277 merger.RemoveSources(importer_source); - return ret; + if (ret_or_error)

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

jingham added a comment. IIUC, when Expected returns fails, it returns an Error object that might have information about what went wrong. Would it be possible to include the contents of that error n the log message? We often get "I can't run an expression in a really complex proprietary code

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

martong updated this revision to Diff 197948. martong added a comment. - Log and do not assert anywhere Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61438/new/ https://reviews.llvm.org/D61438 Files: clang/include/clang/AST/ASTImporter.h

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

teemperor added a comment. I think the best way to handle these errors in LLDB is to just log and then return some default value. That should make the current command print an error, which is better than terminating LLDB. Otherwise the LLDB part of this patch LGTM. Repository: rG LLVM

[PATCH] D61438: [ASTImporter] Use llvm::Expected and Error in the importer API

martong created this revision. martong added reviewers: shafik, teemperor, aprantl, a_sidorin, balazske. Herald added subscribers: lldb-commits, cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs. Herald added a reviewer: a.sidorin. Herald added projects: clang, LLDB. This is the final phase of