[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

This revision was automatically updated to reflect the committed changes. Closed by commit rL367360: [Symbol] Use llvm::Expected when getting TypeSystems (authored by xiaobai, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

JDevlieghere added inline comments. Comment at: source/API/SBModule.cpp:525 module_sp->GetTypeSystemForLanguage(eLanguageTypeC); - if (type_system) { -CompilerType compiler_type = type_system->GetBuiltinTypeByName(name); + if (auto err =

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

xiaobai updated this revision to Diff 212232. xiaobai added a comment. Small logic change in SBModule CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65122/new/ https://reviews.llvm.org/D65122 Files: include/lldb/Core/Module.h include/lldb/Symbol/SymbolFile.h

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

xiaobai added inline comments. Comment at: source/API/SBModule.cpp:525 module_sp->GetTypeSystemForLanguage(eLanguageTypeC); - if (type_system) { -CompilerType compiler_type = type_system->GetBuiltinTypeByName(name); + if (auto err =

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

JDevlieghere added inline comments. Comment at: source/API/SBModule.cpp:525 module_sp->GetTypeSystemForLanguage(eLanguageTypeC); - if (type_system) { -CompilerType compiler_type = type_system->GetBuiltinTypeByName(name); + if (auto err =

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

xiaobai added a comment. In D65122#1604352 , @labath wrote: > I have one remark about the consumeError+LLDB_LOG pattern. As for whether > this is better than status quo or not, I still don't have an opinion on that. > :) Thanks for pointing out

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

xiaobai updated this revision to Diff 212210. xiaobai added a comment. Fix incorrect error logging pattern CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65122/new/ https://reviews.llvm.org/D65122 Files: include/lldb/Core/Module.h include/lldb/Symbol/SymbolFile.h

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

labath added a comment. I have one remark about the consumeError+LLDB_LOG pattern. As for whether this is better than status quo or not, I still don't have an opinion on that. :) Comment at: source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:80-84 +

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

xiaobai updated this revision to Diff 212031. xiaobai added a comment. Address comments: - Use Expected - Did some formatting - Made error checking more explicit CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65122/new/ https://reviews.llvm.org/D65122 Files:

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

xiaobai added a comment. I'm going to update this diff with what I changed to give y'all a better idea of what has been changed. I shoulda done that to begin with... :P In D65122#1602145 , @labath wrote: > In D65122#1602067

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

jingham added a comment. It seems to me part of the goal of Alex's efforts is to make it possible for a given lldb to have or not have support for any particular type system. In some future day they might even be live pluggable, so that which ones get loaded would be a user choice. In that

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

labath added a comment. In D65122#1602067 , @JDevlieghere wrote: > In D65122#1602025 , @xiaobai wrote: > > > After going through this and modifying this patch, I can't help but wonder > > if `llvm::Optional`

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

JDevlieghere added a comment. In D65122#1602025 , @xiaobai wrote: > After going through this and modifying this patch, I can't help but wonder if > `llvm::Optional` would be more appropriate. There are plenty of > instances where it's not a hard error

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

xiaobai added a comment. After going through this and modifying this patch, I can't help but wonder if `llvm::Optional` would be more appropriate. There are plenty of instances where it's not a hard error if you can't get a TypeSystem and the appropriate action is probably just to log and move

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

labath added inline comments. Comment at: source/Core/ValueObjectRegister.cpp:261 + if (auto *exe_module = target->GetExecutableModulePointer()) { +if (auto type_system_or_err = +exe_module->GetTypeSystemForLanguage(eLanguageTypeC)) {

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

xiaobai marked 4 inline comments as done. xiaobai added inline comments. Comment at: source/Core/ValueObjectRegister.cpp:261 + if (auto *exe_module = target->GetExecutableModulePointer()) { +if (auto type_system_or_err = +

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

labath added a comment. Herald added a reviewer: jdoerfert. The intention is for the pointer in `Expected` to be non-null in the success case, right? If that's true, then I'd suggest to make that explicit by returning a reference (`Expected`) instead. Comment at:

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

JDevlieghere added inline comments. Comment at: source/Breakpoint/Watchpoint.cpp:45 + if (log) +log->Printf( +"Watchpoint::Watchpoint(): Failed to set type.\nReason: %s",

[Lldb-commits] [PATCH] D65122: [Symbol] Use llvm::Expected when getting TypeSystems

xiaobai created this revision. xiaobai added reviewers: JDevlieghere, davide, compnerd. Herald added a subscriber: jdoerfert. This commit achieves the following: - Functions used to return a `TypeSystem *` return an `llvm::Expected` now. This means that the result of a call is always checked,