labath added a comment.
Herald added a reviewer: jdoerfert.
The intention is for the pointer in `Expected<TypeSystem *>` 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<TypeSystem&>`) instead.
================
Comment at: source/Core/ValueObjectRegister.cpp:261
+ if (auto *exe_module = target->GetExecutableModulePointer()) {
+ if (auto type_system_or_err =
+ exe_module->GetTypeSystemForLanguage(eLanguageTypeC)) {
----------------
JDevlieghere wrote:
> As a general note: I think it's good practice to use `llvm::consumeError`
> when discarding the error to make it explicit that it's not handled. This
> also makes it easier down the line to handle the error, e.g. by logging it.
This is not just good practice, it is actually required. An simply checking the
bool-ness of the Expected object will not set the "checked" flag of the error
object, and it will assert when it is destroyed (if asserts are enabled).
================
Comment at: source/Core/ValueObjectRegister.cpp:263
+ exe_module->GetTypeSystemForLanguage(eLanguageTypeC)) {
+ auto *type_system = *type_system_or_err;
m_compiler_type = type_system->GetBuiltinTypeForEncodingAndBitSize(
----------------
If you returned a reference then you also wouldn't need this extra variable to
avoid weird double-deref.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65122/new/
https://reviews.llvm.org/D65122
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
