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

[Alex Langford via Phabricator via lldb-commits]

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 =
+                exe_module->GetTypeSystemForLanguage(eLanguageTypeC)) {
----------------
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?


================
Comment at: source/Expression/Materializer.cpp:875
+    if (!type_system_or_err) {
+      err.SetErrorStringWithFormat(
+          "Couldn't dematerialize a result variable: "
----------------
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.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFBaseDIE.cpp:105
 
-lldb_private::TypeSystem *DWARFBaseDIE::GetTypeSystem() const {
-  if (m_cu)
+llvm::Expected<lldb_private::TypeSystem *> DWARFBaseDIE::GetTypeSystem() const {
+  llvm::Error err = llvm::Error::success();
----------------
JDevlieghere wrote:
> Seems like this could be a lot less complex with an early return:
> 
> ```
> if (!m_cu) {
>     return llvm::make_error<llvm::StringError>(
>         "Unable to get TypeSystem, no compilation unit available",
>         llvm::inconvertibleErrorCode());
> }
> return m_cu->GetTypeSystem();
> ```
Hmm, yeah that looks better.


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

https://reviews.llvm.org/D65122