[lldb] Change interface of StructuredData::Array::GetItemAtIndexAsDictionary #71961
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
…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).
|
@llvm/pr-subscribers-lldb Author: Alex Langford (bulbazord) ChangesSimilar to my previous patch (#71613) where I changed
Full diff: https://github.com/llvm/llvm-project/pull/71961.diff 6 Files Affected:
diff --git a/lldb/include/lldb/Utility/StructuredData.h b/lldb/include/lldb/Utility/StructuredData.h
index 6e39bcff2c0af0b..8d0ae372f43c6bf 100644
--- a/lldb/include/lldb/Utility/StructuredData.h
+++ b/lldb/include/lldb/Utility/StructuredData.h
@@ -256,14 +256,24 @@ class StructuredData {
return {};
}
- bool GetItemAtIndexAsDictionary(size_t idx, Dictionary *&result) const {
- result = nullptr;
- ObjectSP value_sp = GetItemAtIndex(idx);
- if (value_sp.get()) {
- result = value_sp->GetAsDictionary();
- return (result != nullptr);
+ /// Retrieves the element at index \a idx from a StructuredData::Array if it
+ /// is a Dictionary.
+ ///
+ /// \param[in] idx
+ /// The index of the element to retrieve.
+ ///
+ /// \return
+ /// If the element at index \a idx is a Dictionary, this method returns a
+ /// valid pointer to the Dictionary wrapped in a std::optional. If the
+ /// element is not a Dictionary or the index is invalid, this returns
+ /// std::nullopt. Note that the underlying Dictionary pointer is never
+ /// nullptr.
+ std::optional<Dictionary *> GetItemAtIndexAsDictionary(size_t idx) const {
+ if (auto item_sp = GetItemAtIndex(idx)) {
+ if (auto *dict = item_sp->GetAsDictionary())
+ return dict;
}
- return false;
+ return {};
}
bool GetItemAtIndexAsArray(size_t idx, Array *&result) const {
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 3714e37ec5c57d0..1ea4f649427727f 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -5688,14 +5688,16 @@ bool ObjectFileMachO::GetCorefileThreadExtraInfos(std::vector<tid_t> &tids) {
}
const size_t num_threads = threads->GetSize();
for (size_t i = 0; i < num_threads; i++) {
- StructuredData::Dictionary *thread;
- if (!threads->GetItemAtIndexAsDictionary(i, thread) || !thread) {
+ std::optional<StructuredData::Dictionary *> maybe_thread =
+ threads->GetItemAtIndexAsDictionary(i);
+ if (!maybe_thread) {
LLDB_LOGF(log,
"Unable to read 'process metadata' LC_NOTE, threads "
"array does not have a dictionary at index %zu.",
i);
return false;
}
+ StructuredData::Dictionary *thread = *maybe_thread;
tid_t tid = LLDB_INVALID_THREAD_ID;
if (thread->GetValueForKeyAsInteger<tid_t>("thread_id", tid))
if (tid == 0)
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
index 04d98b96acd6839..2cf8c29bf9d2fe9 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -2649,10 +2649,12 @@ size_t GDBRemoteCommunicationClient::QueryGDBServer(
return 0;
for (size_t i = 0, count = array->GetSize(); i < count; ++i) {
- StructuredData::Dictionary *element = nullptr;
- if (!array->GetItemAtIndexAsDictionary(i, element))
+ std::optional<StructuredData::Dictionary *> maybe_element =
+ array->GetItemAtIndexAsDictionary(i);
+ if (!maybe_element)
continue;
+ StructuredData::Dictionary *element = *maybe_element;
uint16_t port = 0;
if (StructuredData::ObjectSP port_osp =
element->GetValueForKey(llvm::StringRef("port")))
diff --git a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
index aa2796db15cd00a..88a4ca3b0389fb9 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
+++ b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
@@ -176,8 +176,9 @@ bool ScriptedThread::LoadArtificialStackFrames() {
StackFrameListSP frames = GetStackFrameList();
for (size_t idx = 0; idx < arr_size; idx++) {
- StructuredData::Dictionary *dict;
- if (!arr_sp->GetItemAtIndexAsDictionary(idx, dict) || !dict)
+ std::optional<StructuredData::Dictionary *> maybe_dict =
+ arr_sp->GetItemAtIndexAsDictionary(idx);
+ if (!maybe_dict)
return ScriptedInterface::ErrorWithMessage<bool>(
LLVM_PRETTY_FUNCTION,
llvm::Twine(
@@ -185,6 +186,7 @@ bool ScriptedThread::LoadArtificialStackFrames() {
llvm::Twine(idx) + llvm::Twine(") from stackframe array."))
.str(),
error, LLDBLog::Thread);
+ StructuredData::Dictionary *dict = *maybe_dict;
lldb::addr_t pc;
if (!dict->GetValueForKeyAsInteger("pc", pc))
diff --git a/lldb/source/Target/DynamicRegisterInfo.cpp b/lldb/source/Target/DynamicRegisterInfo.cpp
index cc2df5b97940fed..7469c1d4259afc2 100644
--- a/lldb/source/Target/DynamicRegisterInfo.cpp
+++ b/lldb/source/Target/DynamicRegisterInfo.cpp
@@ -231,13 +231,15 @@ DynamicRegisterInfo::SetRegisterInfo(const StructuredData::Dictionary &dict,
// InvalidateNameMap;
// InvalidateNameMap invalidate_map;
for (uint32_t i = 0; i < num_regs; ++i) {
- StructuredData::Dictionary *reg_info_dict = nullptr;
- if (!regs->GetItemAtIndexAsDictionary(i, reg_info_dict)) {
+ std::optional<StructuredData::Dictionary *> maybe_reg_info_dict =
+ regs->GetItemAtIndexAsDictionary(i);
+ if (!maybe_reg_info_dict) {
Clear();
printf("error: items in the 'registers' array must be dictionaries\n");
regs->DumpToStdout();
return 0;
}
+ StructuredData::Dictionary *reg_info_dict = *maybe_reg_info_dict;
// { 'name':'rcx' , 'bitsize' : 64, 'offset' : 16,
// 'encoding':'uint' , 'format':'hex' , 'set': 0, 'ehframe' : 2,
diff --git a/lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp b/lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
index ed1c8b92b3db74d..32833920ffc57f6 100644
--- a/lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
+++ b/lldb/unittests/tools/lldb-server/tests/MessageObjects.cpp
@@ -106,11 +106,12 @@ Expected<JThreadsInfo> JThreadsInfo::create(StringRef Response,
return make_parsing_error("JThreadsInfo: JSON array");
for (size_t i = 0; i < array->GetSize(); i++) {
- StructuredData::Dictionary *thread_info;
- array->GetItemAtIndexAsDictionary(i, thread_info);
- if (!thread_info)
+ std::optional<StructuredData::Dictionary *> maybe_thread_info =
+ array->GetItemAtIndexAsDictionary(i);
+ if (!maybe_thread_info)
return make_parsing_error("JThreadsInfo: JSON obj at {0}", i);
+ StructuredData::Dictionary *thread_info = *maybe_thread_info;
StringRef name, reason;
thread_info->GetValueForKeyAsString("name", name);
thread_info->GetValueForKeyAsString("reason", reason);
|
JDevlieghere
approved these changes
Nov 10, 2023
|
A great future patch would be to have StructuredData backed by the llvm::json stuff instead of our own custom version of this a JSON style object hiearchy |
I think that I agree. Unfortunately I don't have time to drive that right now and it might appear to not make sense why I'm doing these interface changes now if I think we should refactor this class more thoroughly in the future. That being said, part of my goal in refactoring these interfaces is to force folks to think about error handling more explicitly. The other goal is so I can do other refactors in a safer/nicer way. 🙂 |
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
…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.
Similar to my previous patch (#71613) where I changed
GetItemAtIndexAsString, this patch makes the same change toGetItemAtIndexAsDictionary.GetItemAtIndexAsDictionarynow returns a std::optional that is eitherstd::nulloptor 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).