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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: source/Breakpoint/Watchpoint.cpp:45
+      if (log)
+        log->Printf(
+            "Watchpoint::Watchpoint(): Failed to set type.\nReason: %s",
----------------
```LLDB_LOG(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_WATCHPOINTS), "Watchpoint::Watchpoint(): Failed to set type.\nReason: {}", llvm::toString(type_system_or_err.takeError())```


================
Comment at: source/Core/ValueObjectRegister.cpp:261
+      if (auto *exe_module = target->GetExecutableModulePointer()) {
+        if (auto type_system_or_err =
+                exe_module->GetTypeSystemForLanguage(eLanguageTypeC)) {
----------------
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.


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


================
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();
----------------
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();
```


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:744
   ASSERT_MODULE_LOCK(this);
-  if (die.IsValid()) {
-    TypeSystem *type_system =
-        GetTypeSystemForLanguage(die.GetCU()->GetLanguageType());
-
-    if (type_system) {
-      DWARFASTParser *dwarf_ast = type_system->GetDWARFParser();
-      if (dwarf_ast)
-        return dwarf_ast->ParseFunctionFromDWARF(comp_unit, die);
-    }
+  if (!die.IsValid()) {
+    return nullptr;
----------------
Remove the braces for consistency with the `if`s below. 


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

https://reviews.llvm.org/D65122