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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Tue Jul 23 00:19:53 PDT 2019

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.



More information about the lldb-commits mailing list