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

[Pavel Labath via Phabricator via lldb-commits]

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