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

[Pavel Labath via Phabricator via lldb-commits]

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)) {
----------------
xiaobai wrote:
> labath wrote:
> > 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).
> So what you're saying is do not define it in an if condition, but rather get the Expected and if it isn't valid then we consume the error, else use the type system. Is that right?
Yes, although technically, the value can still be defined in the if-condition, as its scope extends into the else clause. So the following code is perfectly valid (if slightly surprising):
```
if (auto expected_foo = maybe_get_foo())
  do_stuff(*expected_foo);
else
  log(expected_foo.takeError());
```


================
Comment at: source/Expression/Materializer.cpp:875
+    if (!type_system_or_err) {
+      err.SetErrorStringWithFormat(
+          "Couldn't dematerialize a result variable: "
----------------
xiaobai wrote:
> JDevlieghere wrote:
> > Another possible alternative would be to join the errors (llvm::joinErrors) and use the resulting error to initialize the status. Not sure if that makes things better here though. 
> I'm not sure either, I'll try it and see what it looks like.
I'm not a fan of joined errors, and this does not sound like the right place to start using them, as these aren't two separate errors that we've encountered, but rather one error that ends up causing another error to happen. So the best way to model this IMO would be to use nested errors the same way that Java has nested exceptions  (but I don't think it's worth doing that here).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65122/new/

https://reviews.llvm.org/D65122