[lldb][NFCI] Remove BreakpointIDList::InsertStringArray

[bulbazord]

This abstraction is leaky and BreakpointIDList does not need to know about CommandReturnObject.
Additionally, setting the CommandReturnObject inout param to a success state does very little. The function returns immediately if the input ArrayRef is empty, and reading
CommandObjectMultiwordBreakpoint::VerifyIDs more closely, the input is always empty if the previous call to BreakpointIDList::FindAndReplaceIDRanges failed. If the call was successful, then the CommandReturnObject is already in a success state.

I have opted to remove the function altogether and inline the functionality where it was used.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Alex Langford (bulbazord)

Changes

This abstraction is leaky and BreakpointIDList does not need to know about CommandReturnObject.
Additionally, setting the CommandReturnObject inout param to a success state does very little. The function returns immediately if the input ArrayRef is empty, and reading
CommandObjectMultiwordBreakpoint::VerifyIDs more closely, the input is always empty if the previous call to BreakpointIDList::FindAndReplaceIDRanges failed. If the call was successful, then the CommandReturnObject is already in a success state.

I have opted to remove the function altogether and inline the functionality where it was used.

---

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

3 Files Affected:

• (modified) lldb/include/lldb/Breakpoint/BreakpointIDList.h (-3)
• (modified) lldb/source/Breakpoint/BreakpointIDList.cpp (-13)
• (modified) lldb/source/Commands/CommandObjectBreakpoint.cpp (+3-1)

diff --git a/lldb/include/lldb/Breakpoint/BreakpointIDList.h b/lldb/include/lldb/Breakpoint/BreakpointIDList.h
index 924cb1f26b8b8a..161d1a2d314ead 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointIDList.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointIDList.h
@@ -48,9 +48,6 @@ class BreakpointIDList {
 
   bool FindBreakpointID(const char *bp_id, size_t *position) const;
 
-  void InsertStringArray(llvm::ArrayRef<const char *> string_array,
-                         CommandReturnObject &result);
-
   // Returns a pair consisting of the beginning and end of a breakpoint
   // ID range expression.  If the input string is not a valid specification,
   // returns an empty pair.
diff --git a/lldb/source/Breakpoint/BreakpointIDList.cpp b/lldb/source/Breakpoint/BreakpointIDList.cpp
index dd16d3b6388c46..c4fdbc370b223d 100644
--- a/lldb/source/Breakpoint/BreakpointIDList.cpp
+++ b/lldb/source/Breakpoint/BreakpointIDList.cpp
@@ -82,19 +82,6 @@ bool BreakpointIDList::FindBreakpointID(const char *bp_id_str,
   return FindBreakpointID(*bp_id, position);
 }
 
-void BreakpointIDList::InsertStringArray(
-    llvm::ArrayRef<const char *> string_array, CommandReturnObject &result) {
-  if(string_array.empty())
-    return;
-
-  for (const char *str : string_array) {
-    auto bp_id = BreakpointID::ParseCanonicalReference(str);
-    if (bp_id)
-      m_breakpoint_ids.push_back(*bp_id);
-  }
-  result.SetStatus(eReturnStatusSuccessFinishNoResult);
-}
-
 //  This function takes OLD_ARGS, which is usually the result of breaking the
 //  command string arguments into
 //  an array of space-separated strings, and searches through the arguments for
diff --git a/lldb/source/Commands/CommandObjectBreakpoint.cpp b/lldb/source/Commands/CommandObjectBreakpoint.cpp
index 63492590d32d66..f9ba68eda3ff1f 100644
--- a/lldb/source/Commands/CommandObjectBreakpoint.cpp
+++ b/lldb/source/Commands/CommandObjectBreakpoint.cpp
@@ -2494,7 +2494,9 @@ void CommandObjectMultiwordBreakpoint::VerifyIDs(
   // NOW, convert the list of breakpoint id strings in TEMP_ARGS into an actual
   // BreakpointIDList:
 
-  valid_ids->InsertStringArray(temp_args.GetArgumentArrayRef(), result);
+  for (llvm::StringRef temp_arg : temp_args.GetArgumentArrayRef())
+    if (auto bp_id = BreakpointID::ParseCanonicalReference(temp_arg))
+      valid_ids->AddBreakpointID(*bp_id);
 
   // At this point,  all of the breakpoint ids that the user passed in have
   // been converted to breakpoint IDs and put into valid_ids.

[felipepiovezan]

LGTM!

I see that we still have FindAndReplaceIDRanges referencing CommandReturnObject; out of curiosity, have you had a chance to look into removing that one (if it makes sense)? I see that CommandReturnObject is neither #includeed nor forward declared in the header, so the code is very much relying on others files to do either of those.

[bulbazord]

LGTM!

I see that we still have FindAndReplaceIDRanges referencing CommandReturnObject; out of curiosity, have you had a chance to look into removing that one (if it makes sense)? I see that CommandReturnObject is neither #includeed nor forward declared in the header, so the code is very much relying on others files to do either of those.

Yes, I planned on doing that in a follow-up. :)