[lldb] Change interface of StructuredData::Array::GetItemAtIndexAsString #71613
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
bulbazord
requested review from
JDevlieghere,
DavidSpickett,
clayborg,
medismailben,
jimingham,
jasonmolenda and
chelcassanova
|
@llvm/pr-subscribers-lldb Author: Alex Langford (bulbazord) ChangesThis patch changes the interface of Full diff: https://github.com/llvm/llvm-project/pull/71613.diff 9 Files Affected:
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))
|
|
LGTM, happy to see more bool + parameter -> optional conversions. |
DavidSpickett
approved these changes
Nov 8, 2023
bulbazord
added a commit
to bulbazord/llvm-project
that referenced
this pull request
Nov 10, 2023
…tionary Similar to my previous patch (llvm#71613) where I changed `GetItemAtIndexAsString`, this patch makes the same change to `GetItemAtIndexAsDictionary`. `GetItemAtIndexAsDictionary` now returns a std::optional that is either `std::nullopt` or is a valid pointer. Therefore, if the optional is populated, we consider the pointer to always be valid (i.e. no need to check pointer validity).
bulbazord
added a commit
that referenced
this pull request
Nov 10, 2023
…tionary (#71961) Similar to my previous patch (#71613) where I changed `GetItemAtIndexAsString`, this patch makes the same change to `GetItemAtIndexAsDictionary`. `GetItemAtIndexAsDictionary` now returns a std::optional that is either `std::nullopt` or is a valid pointer. Therefore, if the optional is populated, we consider the pointer to always be valid (i.e. no need to check pointer validity).
bulbazord
added a commit
to bulbazord/llvm-project
that referenced
this pull request
Nov 10, 2023
…eger This is a follow-up to (llvm#71613) and (llvm#71961).
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
…ing (llvm#71613) 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.
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
…tionary (llvm#71961) Similar to my previous patch (llvm#71613) where I changed `GetItemAtIndexAsString`, this patch makes the same change to `GetItemAtIndexAsDictionary`. `GetItemAtIndexAsDictionary` now returns a std::optional that is either `std::nullopt` or is a valid pointer. Therefore, if the optional is populated, we consider the pointer to always be valid (i.e. no need to check pointer validity).
justinfargnoli
pushed a commit
to justinfargnoli/llvm-project
that referenced
this pull request
Jan 28, 2024
…eger (llvm#71993) This is a follow-up to (llvm#71613) and (llvm#71961).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.