[Lldb-commits] [lldb] [lldb] Change interface of StructuredData::Array::GetItemAtIndexAsString (PR #71613)

via lldb-commits lldb-commits at lists.llvm.org
Tue Nov 7 17:02:10 PST 2023


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Alex Langford (bulbazord)

<details>
<summary>Changes</summary>

This patch changes the interface of
StructuredData::Array::GetItemAtIndexAsString to return a `std::optional<llvm::StringRef>` instead of taking an out parameter. More generally, this commit serves as proposal that we change all of the sibling APIs (`GetItemAtIndexAs`) to do the same thing. The reason this isn't one giant patch is because it is rather unwieldy changing just one of these, so if this is approved, I will do all of the other ones as individual follow-ups.

---
Full diff: https://github.com/llvm/llvm-project/pull/71613.diff


9 Files Affected:

- (modified) lldb/include/lldb/Utility/StructuredData.h (+6-16) 
- (modified) lldb/source/Breakpoint/Breakpoint.cpp (+7-9) 
- (modified) lldb/source/Breakpoint/BreakpointOptions.cpp (+3-4) 
- (modified) lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp (+4-4) 
- (modified) lldb/source/Breakpoint/BreakpointResolverName.cpp (+4-5) 
- (modified) lldb/source/Commands/CommandObjectBreakpoint.cpp (+3-3) 
- (modified) lldb/source/Core/SearchFilter.cpp (+16-16) 
- (modified) lldb/source/Target/DynamicRegisterInfo.cpp (+14-12) 
- (modified) lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp (+6-2) 


``````````diff
diff --git a/lldb/include/lldb/Utility/StructuredData.h b/lldb/include/lldb/Utility/StructuredData.h
index 279238f9af76e01..6e39bcff2c0af0b 100644
--- a/lldb/include/lldb/Utility/StructuredData.h
+++ b/lldb/include/lldb/Utility/StructuredData.h
@@ -23,6 +23,7 @@
 #include <functional>
 #include <map>
 #include <memory>
+#include <optional>
 #include <string>
 #include <type_traits>
 #include <utility>
@@ -247,23 +248,12 @@ class StructuredData {
       return success;
     }
 
-    bool GetItemAtIndexAsString(size_t idx, llvm::StringRef &result) const {
-      ObjectSP value_sp = GetItemAtIndex(idx);
-      if (value_sp.get()) {
-        if (auto string_value = value_sp->GetAsString()) {
-          result = string_value->GetValue();
-          return true;
-        }
+    std::optional<llvm::StringRef> GetItemAtIndexAsString(size_t idx) const {
+      if (auto item_sp = GetItemAtIndex(idx)) {
+        if (auto *string_value = item_sp->GetAsString())
+          return string_value->GetValue();
       }
-      return false;
-    }
-
-    bool GetItemAtIndexAsString(size_t idx, llvm::StringRef &result,
-                                llvm::StringRef default_val) const {
-      bool success = GetItemAtIndexAsString(idx, result);
-      if (!success)
-        result = default_val;
-      return success;
+      return {};
     }
 
     bool GetItemAtIndexAsDictionary(size_t idx, Dictionary *&result) const {
diff --git a/lldb/source/Breakpoint/Breakpoint.cpp b/lldb/source/Breakpoint/Breakpoint.cpp
index ff4f195ea30909e..28a6a16d45907ef 100644
--- a/lldb/source/Breakpoint/Breakpoint.cpp
+++ b/lldb/source/Breakpoint/Breakpoint.cpp
@@ -205,10 +205,9 @@ lldb::BreakpointSP Breakpoint::CreateFromStructuredData(
   if (success && names_array) {
     size_t num_names = names_array->GetSize();
     for (size_t i = 0; i < num_names; i++) {
-      llvm::StringRef name;
-      Status error;
-      success = names_array->GetItemAtIndexAsString(i, name);
-      target.AddNameToBreakpoint(result_sp, name.str().c_str(), error);
+      if (std::optional<llvm::StringRef> maybe_name =
+              names_array->GetItemAtIndexAsString(i))
+        target.AddNameToBreakpoint(result_sp, *maybe_name, error);
     }
   }
 
@@ -238,11 +237,10 @@ bool Breakpoint::SerializedBreakpointMatchesNames(
   size_t num_names = names_array->GetSize();
 
   for (size_t i = 0; i < num_names; i++) {
-    llvm::StringRef name;
-    if (names_array->GetItemAtIndexAsString(i, name)) {
-      if (llvm::is_contained(names, name))
-        return true;
-    }
+    std::optional<llvm::StringRef> maybe_name =
+        names_array->GetItemAtIndexAsString(i);
+    if (maybe_name && llvm::is_contained(names, *maybe_name))
+      return true;
   }
   return false;
 }
diff --git a/lldb/source/Breakpoint/BreakpointOptions.cpp b/lldb/source/Breakpoint/BreakpointOptions.cpp
index 268c52341dfed6e..6c6037dd9edd3a4 100644
--- a/lldb/source/Breakpoint/BreakpointOptions.cpp
+++ b/lldb/source/Breakpoint/BreakpointOptions.cpp
@@ -88,10 +88,9 @@ BreakpointOptions::CommandData::CreateFromStructuredData(
   if (success) {
     size_t num_elems = user_source->GetSize();
     for (size_t i = 0; i < num_elems; i++) {
-      llvm::StringRef elem_string;
-      success = user_source->GetItemAtIndexAsString(i, elem_string);
-      if (success)
-        data_up->user_source.AppendString(elem_string);
+      if (std::optional<llvm::StringRef> maybe_elem_string =
+              user_source->GetItemAtIndexAsString(i))
+        data_up->user_source.AppendString(*maybe_elem_string);
     }
   }
 
diff --git a/lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp b/lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp
index 38de16a56155e96..13c7f17fd807ee5 100644
--- a/lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp
+++ b/lldb/source/Breakpoint/BreakpointResolverFileRegex.cpp
@@ -56,14 +56,14 @@ BreakpointResolverSP BreakpointResolverFileRegex::CreateFromStructuredData(
   if (success && names_array) {
     size_t num_names = names_array->GetSize();
     for (size_t i = 0; i < num_names; i++) {
-      llvm::StringRef name;
-      success = names_array->GetItemAtIndexAsString(i, name);
-      if (!success) {
+      std::optional<llvm::StringRef> maybe_name =
+          names_array->GetItemAtIndexAsString(i);
+      if (!maybe_name) {
         error.SetErrorStringWithFormat(
             "BRFR::CFSD: Malformed element %zu in the names array.", i);
         return nullptr;
       }
-      names_set.insert(std::string(name));
+      names_set.insert(std::string(*maybe_name));
     }
   }
 
diff --git a/lldb/source/Breakpoint/BreakpointResolverName.cpp b/lldb/source/Breakpoint/BreakpointResolverName.cpp
index ef61df51ba16600..0097046cf511b5d 100644
--- a/lldb/source/Breakpoint/BreakpointResolverName.cpp
+++ b/lldb/source/Breakpoint/BreakpointResolverName.cpp
@@ -155,10 +155,9 @@ BreakpointResolverSP BreakpointResolverName::CreateFromStructuredData(
     std::vector<std::string> names;
     std::vector<FunctionNameType> name_masks;
     for (size_t i = 0; i < num_elem; i++) {
-      llvm::StringRef name;
-
-      success = names_array->GetItemAtIndexAsString(i, name);
-      if (!success) {
+      std::optional<llvm::StringRef> maybe_name =
+          names_array->GetItemAtIndexAsString(i);
+      if (!maybe_name) {
         error.SetErrorString("BRN::CFSD: name entry is not a string.");
         return nullptr;
       }
@@ -168,7 +167,7 @@ BreakpointResolverSP BreakpointResolverName::CreateFromStructuredData(
         error.SetErrorString("BRN::CFSD: name mask entry is not an integer.");
         return nullptr;
       }
-      names.push_back(std::string(name));
+      names.push_back(std::string(*maybe_name));
       name_masks.push_back(static_cast<FunctionNameType>(fnt));
     }
 
diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp b/lldb/source/Commands/CommandObjectBreakpoint.cpp
index e1d1c5e42c32a03..63492590d32d665 100644
--- a/lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp
@@ -2235,9 +2235,9 @@ class CommandObjectBreakpointRead : public CommandObjectParsed {
           size_t num_names = names_array->GetSize();
 
           for (size_t i = 0; i < num_names; i++) {
-            llvm::StringRef name;
-            if (names_array->GetItemAtIndexAsString(i, name))
-              request.TryCompleteCurrentArg(name);
+            if (std::optional<llvm::StringRef> maybe_name =
+                    names_array->GetItemAtIndexAsString(i))
+              request.TryCompleteCurrentArg(*maybe_name);
           }
         }
       }
diff --git a/lldb/source/Core/SearchFilter.cpp b/lldb/source/Core/SearchFilter.cpp
index 4f9519b5cc9a784..b3ff73bbf58f04a 100644
--- a/lldb/source/Core/SearchFilter.cpp
+++ b/lldb/source/Core/SearchFilter.cpp
@@ -471,13 +471,13 @@ SearchFilterSP SearchFilterByModule::CreateFromStructuredData(
     return nullptr;
   }
 
-  llvm::StringRef module;
-  success = modules_array->GetItemAtIndexAsString(0, module);
-  if (!success) {
+  std::optional<llvm::StringRef> maybe_module =
+      modules_array->GetItemAtIndexAsString(0);
+  if (!maybe_module) {
     error.SetErrorString("SFBM::CFSD: filter module item not a string.");
     return nullptr;
   }
-  FileSpec module_spec(module);
+  FileSpec module_spec(*maybe_module);
 
   return std::make_shared<SearchFilterByModule>(target_sp, module_spec);
 }
@@ -596,14 +596,14 @@ SearchFilterSP SearchFilterByModuleList::CreateFromStructuredData(
   FileSpecList modules;
   size_t num_modules = modules_array->GetSize();
   for (size_t i = 0; i < num_modules; i++) {
-    llvm::StringRef module;
-    success = modules_array->GetItemAtIndexAsString(i, module);
-    if (!success) {
+    std::optional<llvm::StringRef> maybe_module =
+        modules_array->GetItemAtIndexAsString(i);
+    if (!maybe_module) {
       error.SetErrorStringWithFormat(
           "SFBM::CFSD: filter module item %zu not a string.", i);
       return nullptr;
     }
-    modules.EmplaceBack(module);
+    modules.EmplaceBack(*maybe_module);
   }
   return std::make_shared<SearchFilterByModuleList>(target_sp, modules);
 }
@@ -644,14 +644,14 @@ lldb::SearchFilterSP SearchFilterByModuleListAndCU::CreateFromStructuredData(
   if (success) {
     size_t num_modules = modules_array->GetSize();
     for (size_t i = 0; i < num_modules; i++) {
-      llvm::StringRef module;
-      success = modules_array->GetItemAtIndexAsString(i, module);
-      if (!success) {
+      std::optional<llvm::StringRef> maybe_module =
+          modules_array->GetItemAtIndexAsString(i);
+      if (!maybe_module) {
         error.SetErrorStringWithFormat(
             "SFBM::CFSD: filter module item %zu not a string.", i);
         return result_sp;
       }
-      modules.EmplaceBack(module);
+      modules.EmplaceBack(*maybe_module);
     }
   }
 
@@ -666,14 +666,14 @@ lldb::SearchFilterSP SearchFilterByModuleListAndCU::CreateFromStructuredData(
   size_t num_cus = cus_array->GetSize();
   FileSpecList cus;
   for (size_t i = 0; i < num_cus; i++) {
-    llvm::StringRef cu;
-    success = cus_array->GetItemAtIndexAsString(i, cu);
-    if (!success) {
+    std::optional<llvm::StringRef> maybe_cu =
+        cus_array->GetItemAtIndexAsString(i);
+    if (!maybe_cu) {
       error.SetErrorStringWithFormat(
           "SFBM::CFSD: filter CU item %zu not a string.", i);
       return nullptr;
     }
-    cus.EmplaceBack(cu);
+    cus.EmplaceBack(*maybe_cu);
   }
 
   return std::make_shared<SearchFilterByModuleListAndCU>(
diff --git a/lldb/source/Target/DynamicRegisterInfo.cpp b/lldb/source/Target/DynamicRegisterInfo.cpp
index 0372fd48feae687..cc2df5b97940fed 100644
--- a/lldb/source/Target/DynamicRegisterInfo.cpp
+++ b/lldb/source/Target/DynamicRegisterInfo.cpp
@@ -144,20 +144,21 @@ llvm::Expected<uint32_t> DynamicRegisterInfo::ByteOffsetFromComposite(
   uint32_t composite_offset = UINT32_MAX;
   for (uint32_t composite_idx = 0; composite_idx < num_composite_regs;
        ++composite_idx) {
-    llvm::StringRef composite_reg_name;
-    if (!composite_reg_list.GetItemAtIndexAsString(composite_idx, composite_reg_name))
+    std::optional<llvm::StringRef> maybe_composite_reg_name =
+        composite_reg_list.GetItemAtIndexAsString(composite_idx);
+    if (!maybe_composite_reg_name)
       return llvm::createStringError(
           llvm::inconvertibleErrorCode(),
           "\"composite\" list value is not a Python string at index %d",
           composite_idx);
 
     const RegisterInfo *composite_reg_info =
-        GetRegisterInfo(composite_reg_name);
+        GetRegisterInfo(*maybe_composite_reg_name);
     if (!composite_reg_info)
       return llvm::createStringError(
           llvm::inconvertibleErrorCode(),
           "failed to find composite register by name: \"%s\"",
-          composite_reg_name.str().c_str());
+          maybe_composite_reg_name->str().c_str());
 
     composite_offset =
         std::min(composite_offset, composite_reg_info->byte_offset);
@@ -205,10 +206,11 @@ DynamicRegisterInfo::SetRegisterInfo(const StructuredData::Dictionary &dict,
   if (dict.GetValueForKeyAsArray("sets", sets)) {
     const uint32_t num_sets = sets->GetSize();
     for (uint32_t i = 0; i < num_sets; ++i) {
-      llvm::StringRef set_name;
-      if (sets->GetItemAtIndexAsString(i, set_name) && !set_name.empty()) {
+      std::optional<llvm::StringRef> maybe_set_name =
+          sets->GetItemAtIndexAsString(i);
+      if (maybe_set_name && !maybe_set_name->empty()) {
         m_sets.push_back(
-            {ConstString(set_name).AsCString(), nullptr, 0, nullptr});
+            {ConstString(*maybe_set_name).AsCString(), nullptr, 0, nullptr});
       } else {
         Clear();
         printf("error: register sets must have valid names\n");
@@ -345,12 +347,12 @@ DynamicRegisterInfo::SetRegisterInfo(const StructuredData::Dictionary &dict,
       const size_t num_regs = invalidate_reg_list->GetSize();
       if (num_regs > 0) {
         for (uint32_t idx = 0; idx < num_regs; ++idx) {
-          llvm::StringRef invalidate_reg_name;
           uint64_t invalidate_reg_num;
-          if (invalidate_reg_list->GetItemAtIndexAsString(
-                  idx, invalidate_reg_name)) {
+          std::optional<llvm::StringRef> maybe_invalidate_reg_name =
+              invalidate_reg_list->GetItemAtIndexAsString(idx);
+          if (maybe_invalidate_reg_name) {
             const RegisterInfo *invalidate_reg_info =
-                GetRegisterInfo(invalidate_reg_name);
+                GetRegisterInfo(*maybe_invalidate_reg_name);
             if (invalidate_reg_info) {
               m_invalidate_regs_map[i].push_back(
                   invalidate_reg_info->kinds[eRegisterKindLLDB]);
@@ -359,7 +361,7 @@ DynamicRegisterInfo::SetRegisterInfo(const StructuredData::Dictionary &dict,
               // format
               printf("error: failed to find a 'invalidate-regs' register for "
                      "\"%s\" while parsing register \"%s\"\n",
-                     invalidate_reg_name.str().c_str(), reg_info.name);
+                     maybe_invalidate_reg_name->str().c_str(), reg_info.name);
             }
           } else if (invalidate_reg_list->GetItemAtIndexAsInteger(
                          idx, invalidate_reg_num)) {
diff --git a/lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp b/lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
index da4dc10d4b87c2a..ed1c8b92b3db74d 100644
--- a/lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
+++ b/lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
@@ -76,8 +76,12 @@ JThreadsInfo::parseRegisters(const StructuredData::Dictionary &Dict,
   auto KeysObj = Dict.GetKeys();
   auto Keys = KeysObj->GetAsArray();
   for (size_t i = 0; i < Keys->GetSize(); i++) {
-    StringRef KeyStr, ValueStr;
-    Keys->GetItemAtIndexAsString(i, KeyStr);
+    std::optional<StringRef> MaybeKeyStr = Keys->GetItemAtIndexAsString(i);
+    if (!MaybeKeyStr)
+      return make_parsing_error("JThreadsInfo: Invalid Key at index {0}", i);
+
+    StringRef KeyStr = *MaybeKeyStr;
+    StringRef ValueStr;
     Dict.GetValueForKeyAsString(KeyStr, ValueStr);
     unsigned int Register;
     if (!llvm::to_integer(KeyStr, Register, 10))

``````````

</details>


https://github.com/llvm/llvm-project/pull/71613


More information about the lldb-commits mailing list