Convert some Breakpoint to StringRef

[zturner]

Summary

This is an incremental step towards getting some other code converted. In any case, I believe this makes the breakpoint code significantly easier to understand and also removes many string copies in the range id code.

[jingham]

This looks correct to me. I had a couple of trivial inline comments, but this looks fine.

source/Breakpoint/BreakpointID.cpp

75–76

This really should return a BreakpointID not pointers to the break & loc ID's. It always gets used that way, not sure why it wasn't written that way... That's not a necessary change but while you are cleaning this stuff up...

source/Breakpoint/BreakpointIDList.cpp

329–339

Can you add a comment here saying what is returned, particularly that you return a pair of empty string refs when it isn't an ID range?

[zturner]

Fixed up ParseCanonicalReference and SplitIDRangeExpression as suggested. I could have probably returned an Optional<std::pair<StringRef,StringRef>> but it seemed like overkill. An empty pair seems sufficient for conveying failure.

[jingham]

No good deed goes unpunished... You made ParseCanonicalReference more beautiful but forgot to update the header doc.

Also I didn't see the comment for SplitIDRangeExpression.

include/lldb/Breakpoint/BreakpointID.h

74–75

You changed the interface but not the header doc for it.

source/Breakpoint/BreakpointIDList.cpp

329–330

Did you upload the latest version of your patch, I don't see a comment here...

[zturner]

source/Breakpoint/BreakpointIDList.cpp

329–330

I put the comment in the header. I can put it here if you prefer. Putting it on both places seems unnecessary though. LMK which you prefer. I'll fix up the header doc of the other function in the meantime.

include/lldb/Breakpoint/BreakpointID.h

 
 // C Includes
 // C++ Includes
 // Other libraries and framework includes
 // Project includes
 
 #include "lldb/lldb-private.h"
 
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 
 namespace lldb_private {
 
 //----------------------------------------------------------------------
 // class BreakpointID
 //----------------------------------------------------------------------
 
public:
   void SetBreakpointID(lldb::break_id_t bp_id) { m_break_id = bp_id; }
 
   void SetBreakpointLocationID(lldb::break_id_t loc_id) {
     m_location_id = loc_id;
   }
 
   void GetDescription(Stream *s, lldb::DescriptionLevel level);
 
-  static bool IsRangeIdentifier(const char *str);
-
-  static bool IsValidIDExpression(const char *str);
-
-  static const char *g_range_specifiers[];
+  static bool IsRangeIdentifier(llvm::StringRef str);
+  static bool IsValidIDExpression(llvm::StringRef str);
+  static llvm::ArrayRef<llvm::StringRef> GetRangeSpecifiers();
 
   //------------------------------------------------------------------
   /// Takes an input string containing the description of a breakpoint or
   /// breakpoint and location
   /// and returns the breakpoint ID and the breakpoint location id.
   ///
   /// @param[in] input
   ///     A string containing JUST the breakpoint description.
   /// @param[out] break_id
   ///     This is the break id.
   /// @param[out] break_loc_id
   ///     This is breakpoint location id, or LLDB_INVALID_BREAK_ID is no
   ///     location was specified.
   /// @return
   ///     \b true if the call was able to extract a breakpoint location from the
   ///     string.  \b false otherwise.
   //------------------------------------------------------------------
-  static bool ParseCanonicalReference(const char *input,
-                                      lldb::break_id_t *break_id,
-                                      lldb::break_id_t *break_loc_id);
+  static llvm::Optional<BreakpointID>
+  ParseCanonicalReference(llvm::StringRef input);
 
   //------------------------------------------------------------------
   /// Takes an input string and checks to see whether it is a breakpoint name.
   /// If it is a mal-formed breakpoint name, error will be set to an appropriate
   /// error string.
   ///
   /// @param[in] input
   ///     A string containing JUST the breakpoint description.

include/lldb/Breakpoint/BreakpointIDList.h

 //===-- BreakpointIDList.h --------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_BreakpointIDList_h_
 #define liblldb_BreakpointIDList_h_
 
 // C Includes
 // C++ Includes
+#include <utility>
 #include <vector>
 
 // Other libraries and framework includes
 // Project includes
 
 #include "lldb/Breakpoint/BreakpointID.h"
 #include "lldb/lldb-private.h"
 
 namespace lldb_private {
 
 //----------------------------------------------------------------------
 // class BreakpointIDList
 //----------------------------------------------------------------------
 
 class BreakpointIDList {
 public:
+  // TODO: Convert this class to StringRef.
   typedef std::vector<BreakpointID> BreakpointIDArray;
 
   BreakpointIDList();
 
   virtual ~BreakpointIDList();
 
   size_t GetSize() const;
 
   const BreakpointID &GetBreakpointIDAtIndex(size_t index) const;
 
   bool RemoveBreakpointIDAtIndex(size_t index);
 
   void Clear();
 
   bool AddBreakpointID(BreakpointID bp_id);
 
   bool AddBreakpointID(const char *bp_id);
 
+  // TODO: This should take a const BreakpointID.
   bool FindBreakpointID(BreakpointID &bp_id, size_t *position) const;
 
   bool FindBreakpointID(const char *bp_id, size_t *position) const;
 
   void InsertStringArray(const char **string_array, size_t array_size,
                          CommandReturnObject &result);
 
-  static bool StringContainsIDRangeExpression(const char *in_string,
-                                              size_t *range_start_len,
-                                              size_t *range_end_pos);
+  // 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.
+  static std::pair<llvm::StringRef, llvm::StringRef>
+  SplitIDRangeExpression(llvm::StringRef in_string);
 
   static void FindAndReplaceIDRanges(Args &old_args, Target *target,
                                      bool allow_locations,
                                      CommandReturnObject &result,
                                      Args &new_args);
 
 private:
   BreakpointIDArray m_breakpoint_ids;
   BreakpointID m_invalid_id;
 
   DISALLOW_COPY_AND_ASSIGN(BreakpointIDList);
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_BreakpointIDList_h_

source/Breakpoint/BreakpointID.cpp

 using namespace lldb;
 using namespace lldb_private;
 
 BreakpointID::BreakpointID(break_id_t bp_id, break_id_t loc_id)
     : m_break_id(bp_id), m_location_id(loc_id) {}
 
 BreakpointID::~BreakpointID() = default;
 
-const char *BreakpointID::g_range_specifiers[] = {"-", "to", "To", "TO",
-                                                  nullptr};
+static llvm::StringRef g_range_specifiers[] = {"-", "to", "To", "TO"};
 
 // Tells whether or not STR is valid to use between two strings representing
 // breakpoint IDs, to
 // indicate a range of breakpoint IDs.  This is broken out into a separate
 // function so that we can
 // easily change or add to the format for specifying ID ranges at a later date.
 
-bool BreakpointID::IsRangeIdentifier(const char *str) {
-  int specifier_count = 0;
-  for (int i = 0; g_range_specifiers[i] != nullptr; ++i)
-    ++specifier_count;
-
-  for (int i = 0; i < specifier_count; ++i) {
-    if (strcmp(g_range_specifiers[i], str) == 0)
+bool BreakpointID::IsRangeIdentifier(llvm::StringRef str) {
+  for (auto spec : g_range_specifiers) {
+    if (spec == str)
       return true;
   }
 
   return false;
 }
 
-bool BreakpointID::IsValidIDExpression(const char *str) {
-  break_id_t bp_id;
-  break_id_t loc_id;
-  BreakpointID::ParseCanonicalReference(str, &bp_id, &loc_id);
+bool BreakpointID::IsValidIDExpression(llvm::StringRef str) {
+  return BreakpointID::ParseCanonicalReference(str).hasValue();
+}
 
-  return (bp_id != LLDB_INVALID_BREAK_ID);
+llvm::ArrayRef<llvm::StringRef> BreakpointID::GetRangeSpecifiers() {
+  return g_range_specifiers;
 }
 
 void BreakpointID::GetDescription(Stream *s, lldb::DescriptionLevel level) {
   if (level == eDescriptionLevelVerbose)
     s->Printf("%p BreakpointID:", static_cast<void *>(this));
 
   if (m_break_id == LLDB_INVALID_BREAK_ID)
     s->PutCString("<invalid>");
   else if (m_location_id == LLDB_INVALID_BREAK_ID)
     s->Printf("%i", m_break_id);
   else
     s->Printf("%i.%i", m_break_id, m_location_id);
 }
 
 void BreakpointID::GetCanonicalReference(Stream *s, break_id_t bp_id,
                                          break_id_t loc_id) {
   if (bp_id == LLDB_INVALID_BREAK_ID)
     s->PutCString("<invalid>");
   else if (loc_id == LLDB_INVALID_BREAK_ID)
     s->Printf("%i", bp_id);
   else
     s->Printf("%i.%i", bp_id, loc_id);
 }
 
-bool BreakpointID::ParseCanonicalReference(const char *input,
-                                           break_id_t *break_id_ptr,
-                                           break_id_t *break_loc_id_ptr) {
-  *break_id_ptr = LLDB_INVALID_BREAK_ID;
-  *break_loc_id_ptr = LLDB_INVALID_BREAK_ID;
+llvm::Optional<BreakpointID>
+BreakpointID::ParseCanonicalReference(llvm::StringRef input) {
+  break_id_t bp_id;
+  break_id_t loc_id = LLDB_INVALID_BREAK_ID;
 
-  if (input == nullptr || *input == '\0')
-    return false;
+  if (input.empty())
+    return llvm::None;
 
-  const char *format = "%i%n.%i%n";
-  int chars_consumed_1 = 0;
-  int chars_consumed_2 = 0;
-  int n_items_parsed = ::sscanf(
-      input, format,
-      break_id_ptr,       // %i   parse the breakpoint ID
-      &chars_consumed_1,  // %n   gets the number of characters parsed so far
-      break_loc_id_ptr,   // %i   parse the breakpoint location ID
-      &chars_consumed_2); // %n   gets the number of characters parsed so far
+  // If it doesn't start with an integer, it's not valid.
+  if (input.consumeInteger(0, bp_id))
+    return llvm::None;
 
-  if ((n_items_parsed == 1 && input[chars_consumed_1] == '\0') ||
-      (n_items_parsed == 2 && input[chars_consumed_2] == '\0'))
-    return true;
+  // period is optional, but if it exists, it must be followed by a number.
+  if (input.consume_front(".")) {
+    if (input.consumeInteger(0, loc_id))
+      return llvm::None;
+  }
 
-  // Badly formatted canonical reference.
-  *break_id_ptr = LLDB_INVALID_BREAK_ID;
-  *break_loc_id_ptr = LLDB_INVALID_BREAK_ID;
-  return false;
+  // And at the end, the entire string must have been consumed.
+  if (!input.empty())
+    return llvm::None;
+
+  return BreakpointID(bp_id, loc_id);
 }
 
 bool BreakpointID::StringIsBreakpointName(llvm::StringRef str, Error &error) {
   error.Clear();
   if (str.empty())
     return false;
 
   // First character must be a letter or _

source/Breakpoint/BreakpointIDList.cpp

 bool BreakpointIDList::AddBreakpointID(BreakpointID bp_id) {
   m_breakpoint_ids.push_back(bp_id);
 
   return true; // We don't do any verification in this function, so always
                // return true.
 }
 
 bool BreakpointIDList::AddBreakpointID(const char *bp_id_str) {
-  BreakpointID temp_bp_id;
-  break_id_t bp_id;
-  break_id_t loc_id;
-
-  bool success =
-      BreakpointID::ParseCanonicalReference(bp_id_str, &bp_id, &loc_id);
-
-  if (success) {
-    temp_bp_id.SetID(bp_id, loc_id);
-    m_breakpoint_ids.push_back(temp_bp_id);
-  }
+  auto bp_id = BreakpointID::ParseCanonicalReference(bp_id_str);
+  if (!bp_id.hasValue())
+    return false;
 
-  return success;
+  m_breakpoint_ids.push_back(*bp_id);
 }
 
 bool BreakpointIDList::FindBreakpointID(BreakpointID &bp_id,
                                         size_t *position) const {
   for (size_t i = 0; i < m_breakpoint_ids.size(); ++i) {
     BreakpointID tmp_id = m_breakpoint_ids[i];
     if (tmp_id.GetBreakpointID() == bp_id.GetBreakpointID() &&
         tmp_id.GetLocationID() == bp_id.GetLocationID()) {
       *position = i;
       return true;
     }
   }
 
   return false;
 }
 
 bool BreakpointIDList::FindBreakpointID(const char *bp_id_str,
                                         size_t *position) const {
-  BreakpointID temp_bp_id;
-  break_id_t bp_id;
-  break_id_t loc_id;
-
-  if (BreakpointID::ParseCanonicalReference(bp_id_str, &bp_id, &loc_id)) {
-    temp_bp_id.SetID(bp_id, loc_id);
-    return FindBreakpointID(temp_bp_id, position);
-  } else
+  auto bp_id = BreakpointID::ParseCanonicalReference(bp_id_str);
+  if (!bp_id.hasValue())
     return false;
+
+  return FindBreakpointID(*bp_id, position);
 }
 
 void BreakpointIDList::InsertStringArray(const char **string_array,
                                          size_t array_size,
                                          CommandReturnObject &result) {
   if (string_array == nullptr)
     return;
 
   for (uint32_t i = 0; i < array_size; ++i) {
-    break_id_t bp_id;
-    break_id_t loc_id;
-
-    if (BreakpointID::ParseCanonicalReference(string_array[i], &bp_id,
-                                              &loc_id)) {
-      if (bp_id != LLDB_INVALID_BREAK_ID) {
-        BreakpointID temp_bp_id(bp_id, loc_id);
-        m_breakpoint_ids.push_back(temp_bp_id);
-      } else {
-        result.AppendErrorWithFormat("'%s' is not a valid breakpoint ID.\n",
-                                     string_array[i]);
-        result.SetStatus(eReturnStatusFailed);
-        return;
-      }
-    }
-  }
+    auto bp_id = BreakpointID::ParseCanonicalReference(string_array[i]);
+    if (bp_id.hasValue()) {
+      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
 //  any breakpoint ID range specifiers.
 //  Any string in the array that is not part of an ID range specifier is copied
 //  directly into NEW_ARGS.  If any
 //  ID range specifiers are found, the range is interpreted and a list of
 //  canonical breakpoint IDs corresponding to
 //  all the current breakpoints and locations in the range are added to
 //  NEW_ARGS.  When this function is done,
 //  NEW_ARGS should be a copy of OLD_ARGS, with and ID range specifiers replaced
 //  by the members of the range.
 
 void BreakpointIDList::FindAndReplaceIDRanges(Args &old_args, Target *target,
                                               bool allow_locations,
                                               CommandReturnObject &result,
                                               Args &new_args) {
-  std::string range_start;
-  const char *range_end;
-  const char *current_arg;
+  llvm::StringRef range_from;
+  llvm::StringRef range_to;
+  llvm::StringRef current_arg;
   const size_t num_old_args = old_args.GetArgumentCount();
   std::set<std::string> names_found;
 
   for (size_t i = 0; i < num_old_args; ++i) {
     bool is_range = false;
 
     current_arg = old_args.GetArgumentAtIndex(i);
-    if (!allow_locations && strchr(current_arg, '.') != nullptr) {
+    if (!allow_locations && current_arg.contains('.')) {
       result.AppendErrorWithFormat(
-          "Breakpoint locations not allowed, saw location: %s.", current_arg);
+          "Breakpoint locations not allowed, saw location: %s.",
+          current_arg.str().c_str());
       new_args.Clear();
       return;
     }
 
-    size_t range_start_len = 0;
-    size_t range_end_pos = 0;
+    llvm::StringRef range_expr;
     Error error;
 
-    if (BreakpointIDList::StringContainsIDRangeExpression(
-            current_arg, &range_start_len, &range_end_pos)) {
+    std::tie(range_from, range_to) =
+        BreakpointIDList::SplitIDRangeExpression(current_arg);
+    if (!range_from.empty() && !range_to.empty()) {
       is_range = true;
-      range_start.assign(current_arg, range_start_len);
-      range_end = current_arg + range_end_pos;
-    } else if (BreakpointID::StringIsBreakpointName(
-                   llvm::StringRef(current_arg), error)) {
+    } else if (BreakpointID::StringIsBreakpointName(current_arg, error)) {
       if (!error.Success()) {
         new_args.Clear();
         result.AppendError(error.AsCString());
         result.SetStatus(eReturnStatusFailed);
         return;
       } else
         names_found.insert(current_arg);
     } else if ((i + 2 < num_old_args) &&
                BreakpointID::IsRangeIdentifier(
                    old_args.GetArgumentAtIndex(i + 1)) &&
                BreakpointID::IsValidIDExpression(current_arg) &&
                BreakpointID::IsValidIDExpression(
                    old_args.GetArgumentAtIndex(i + 2))) {
-      range_start.assign(current_arg);
-      range_end = old_args.GetArgumentAtIndex(i + 2);
+      range_from = current_arg;
+      range_to = old_args.GetArgumentAtIndex(i + 2);
       is_range = true;
       i = i + 2;
     } else {
       // See if user has specified id.*
-      std::string tmp_str = old_args.GetArgumentAtIndex(i);
+      llvm::StringRef tmp_str = old_args.GetArgumentAtIndex(i);
       size_t pos = tmp_str.find('.');
-      if (pos != std::string::npos) {
-        std::string bp_id_str = tmp_str.substr(0, pos);
-        if (BreakpointID::IsValidIDExpression(bp_id_str.c_str()) &&
-            tmp_str[pos + 1] == '*' && tmp_str.length() == (pos + 2)) {
-          break_id_t bp_id;
-          break_id_t bp_loc_id;
-
-          BreakpointID::ParseCanonicalReference(bp_id_str.c_str(), &bp_id,
-                                                &bp_loc_id);
-          BreakpointSP breakpoint_sp = target->GetBreakpointByID(bp_id);
+      if (pos != llvm::StringRef::npos) {
+        llvm::StringRef bp_id_str = tmp_str.substr(0, pos);
+        if (BreakpointID::IsValidIDExpression(bp_id_str) &&
+            tmp_str[pos + 1] == '*' && tmp_str.size() == (pos + 2)) {
+
+          BreakpointSP breakpoint_sp;
+          auto bp_id = BreakpointID::ParseCanonicalReference(bp_id_str);
+          if (bp_id.hasValue())
+            breakpoint_sp = target->GetBreakpointByID(bp_id->GetBreakpointID());
           if (!breakpoint_sp) {
             new_args.Clear();
             result.AppendErrorWithFormat("'%d' is not a valid breakpoint ID.\n",
                                          bp_id);
             result.SetStatus(eReturnStatusFailed);
             return;
           }
           const size_t num_locations = breakpoint_sp->GetNumLocations();
           for (size_t j = 0; j < num_locations; ++j) {
             BreakpointLocation *bp_loc =
                 breakpoint_sp->GetLocationAtIndex(j).get();
             StreamString canonical_id_str;
-            BreakpointID::GetCanonicalReference(&canonical_id_str, bp_id,
-                                                bp_loc->GetID());
+            BreakpointID::GetCanonicalReference(
+                &canonical_id_str, bp_id->GetBreakpointID(), bp_loc->GetID());
             new_args.AppendArgument(canonical_id_str.GetString());
           }
         }
       }
     }
 
-    if (is_range) {
-      break_id_t start_bp_id;
-      break_id_t end_bp_id;
-      break_id_t start_loc_id;
-      break_id_t end_loc_id;
-
-      BreakpointID::ParseCanonicalReference(range_start.c_str(), &start_bp_id,
-                                            &start_loc_id);
-      BreakpointID::ParseCanonicalReference(range_end, &end_bp_id, &end_loc_id);
+    if (!is_range) {
+      new_args.AppendArgument(current_arg);
+      continue;
+    }
+
+    auto start_bp = BreakpointID::ParseCanonicalReference(range_from);
+    auto end_bp = BreakpointID::ParseCanonicalReference(range_to);
 
-      if ((start_bp_id == LLDB_INVALID_BREAK_ID) ||
-          (!target->GetBreakpointByID(start_bp_id))) {
-        new_args.Clear();
-        result.AppendErrorWithFormat("'%s' is not a valid breakpoint ID.\n",
-                                     range_start.c_str());
-        result.SetStatus(eReturnStatusFailed);
-        return;
-      }
+    if (!start_bp.hasValue() ||
+        !target->GetBreakpointByID(start_bp->GetBreakpointID())) {
+                                   range_from.str().c_str());
 
-      if ((end_bp_id == LLDB_INVALID_BREAK_ID) ||
-          (!target->GetBreakpointByID(end_bp_id))) {
-        new_args.Clear();
-        result.AppendErrorWithFormat("'%s' is not a valid breakpoint ID.\n",
-                                     range_end);
-        result.SetStatus(eReturnStatusFailed);
-        return;
-      }
-
-      if (((start_loc_id == LLDB_INVALID_BREAK_ID) &&
-           (end_loc_id != LLDB_INVALID_BREAK_ID)) ||
-          ((start_loc_id != LLDB_INVALID_BREAK_ID) &&
-           (end_loc_id == LLDB_INVALID_BREAK_ID))) {
-        new_args.Clear();
-        result.AppendErrorWithFormat("Invalid breakpoint id range:  Either "
-                                     "both ends of range must specify"
-                                     " a breakpoint location, or neither can "
-                                     "specify a breakpoint location.\n");
-        result.SetStatus(eReturnStatusFailed);
-        return;
-      }
+    if (!end_bp.hasValue() ||
+        !target->GetBreakpointByID(end_bp->GetBreakpointID())) {
+                                   range_to.str().c_str());
+    break_id_t start_bp_id = start_bp->GetBreakpointID();
+    break_id_t start_loc_id = start_bp->GetLocationID();
+    break_id_t end_bp_id = end_bp->GetBreakpointID();
+    break_id_t end_loc_id = end_bp->GetLocationID();
+    if ((start_bp_id == LLDB_INVALID_BREAK_ID) &&
+            (end_bp_id != LLDB_INVALID_BREAK_ID) ||
 
-      // We have valid range starting & ending breakpoint IDs.  Go through all
-      // the breakpoints in the
-      // target and find all the breakpoints that fit into this range, and add
-      // them to new_args.
+    // the breakpoints in the target and find all the breakpoints that fit
+    // into this range, and add them to new_args.
 
-      // Next check to see if we have location id's.  If so, make sure the
-      // start_bp_id and end_bp_id are
-      // for the same breakpoint; otherwise we have an illegal range: breakpoint
-      // id ranges that specify
-      // bp locations are NOT allowed to cross major bp id numbers.
+    // start_bp_id and end_bp_id are for the same breakpoint; otherwise we
+    // have an illegal range: breakpoint id ranges that specify bp locations
+    // are NOT allowed to cross major bp id numbers.
 
-      if ((start_loc_id != LLDB_INVALID_BREAK_ID) ||
-          (end_loc_id != LLDB_INVALID_BREAK_ID)) {
-        if (start_bp_id != end_bp_id) {
-          new_args.Clear();
-          result.AppendErrorWithFormat(
-              "Invalid range: Ranges that specify particular breakpoint "
-              "locations"
-              " must be within the same major breakpoint; you specified two"
-              " different major breakpoints, %d and %d.\n",
-              start_bp_id, end_bp_id);
-          result.SetStatus(eReturnStatusFailed);
-          return;
-        }
-      }
 
-      const BreakpointList &breakpoints = target->GetBreakpointList();
-      const size_t num_breakpoints = breakpoints.GetSize();
-      for (size_t j = 0; j < num_breakpoints; ++j) {
-        Breakpoint *breakpoint = breakpoints.GetBreakpointAtIndex(j).get();
-        break_id_t cur_bp_id = breakpoint->GetID();
 
-        if ((cur_bp_id < start_bp_id) || (cur_bp_id > end_bp_id))
-          continue;
 
-        const size_t num_locations = breakpoint->GetNumLocations();
 
-        if ((cur_bp_id == start_bp_id) &&
-            (start_loc_id != LLDB_INVALID_BREAK_ID)) {
-          for (size_t k = 0; k < num_locations; ++k) {
-            BreakpointLocation *bp_loc =
-                breakpoint->GetLocationAtIndex(k).get();
-            if ((bp_loc->GetID() >= start_loc_id) &&
-                (bp_loc->GetID() <= end_loc_id)) {
-              StreamString canonical_id_str;
-              BreakpointID::GetCanonicalReference(&canonical_id_str, cur_bp_id,
-                                                  bp_loc->GetID());
-              new_args.AppendArgument(canonical_id_str.GetString());
-            }
-          }
-        } else if ((cur_bp_id == end_bp_id) &&
-                   (end_loc_id != LLDB_INVALID_BREAK_ID)) {
-          for (size_t k = 0; k < num_locations; ++k) {
-            BreakpointLocation *bp_loc =
-                breakpoint->GetLocationAtIndex(k).get();
-            if (bp_loc->GetID() <= end_loc_id) {
-              StreamString canonical_id_str;
-              BreakpointID::GetCanonicalReference(&canonical_id_str, cur_bp_id,
-                                                  bp_loc->GetID());
-              new_args.AppendArgument(canonical_id_str.GetString());
-            }
-          }
-        } else {
-          StreamString canonical_id_str;
-          BreakpointID::GetCanonicalReference(&canonical_id_str, cur_bp_id,
-                                              LLDB_INVALID_BREAK_ID);
-          new_args.AppendArgument(canonical_id_str.GetString());
-        }
-      }
-    } else // else is_range was false
-    {
-      new_args.AppendArgument(llvm::StringRef::withNullAsEmpty(current_arg));
-    }
+          BreakpointLocation *bp_loc = breakpoint->GetLocationAtIndex(k).get();
+          BreakpointLocation *bp_loc = breakpoint->GetLocationAtIndex(k).get();
   }
 
   // Okay, now see if we found any names, and if we did, add them:
   if (target && names_found.size()) {
     for (BreakpointSP bkpt_sp : target->GetBreakpointList().Breakpoints()) {
       for (std::string name : names_found) {
         if (bkpt_sp->MatchesName(name.c_str())) {
           StreamString canonical_id_str;
           BreakpointID::GetCanonicalReference(
               &canonical_id_str, bkpt_sp->GetID(), LLDB_INVALID_BREAK_ID);
           new_args.AppendArgument(canonical_id_str.GetString());
         }
       }
     }
   }
 
   result.SetStatus(eReturnStatusSuccessFinishNoResult);
 }
 
-bool BreakpointIDList::StringContainsIDRangeExpression(const char *in_string,
-                                                       size_t *range_start_len,
-                                                       size_t *range_end_pos) {
-  bool is_range_expression = false;
-  std::string arg_str = in_string;
-  std::string::size_type idx;
-  std::string::size_type start_pos = 0;
-
-  *range_start_len = 0;
-  *range_end_pos = 0;
-
-  int specifiers_size = 0;
-  for (int i = 0; BreakpointID::g_range_specifiers[i] != nullptr; ++i)
-    ++specifiers_size;
-
-  for (int i = 0; i < specifiers_size && !is_range_expression; ++i) {
-    const char *specifier_str = BreakpointID::g_range_specifiers[i];
-    size_t len = strlen(specifier_str);
-    idx = arg_str.find(BreakpointID::g_range_specifiers[i]);
-    if (idx != std::string::npos) {
-      *range_start_len = idx - start_pos;
-      std::string start_str = arg_str.substr(start_pos, *range_start_len);
-      if (idx + len < arg_str.length()) {
-        *range_end_pos = idx + len;
-        std::string end_str = arg_str.substr(*range_end_pos);
-        if (BreakpointID::IsValidIDExpression(start_str.c_str()) &&
-            BreakpointID::IsValidIDExpression(end_str.c_str())) {
-          is_range_expression = true;
-          //*range_start = start_str;
-          //*range_end = end_str;
-        }
-      }
-    }
-  }
+std::pair<llvm::StringRef, llvm::StringRef>
+BreakpointIDList::SplitIDRangeExpression(llvm::StringRef in_string) {
+  for (auto specifier_str : BreakpointID::GetRangeSpecifiers()) {
+    size_t idx = in_string.find(specifier_str);
+    if (idx == llvm::StringRef::npos)
+      continue;
+    llvm::StringRef right1 = in_string.drop_front(idx);
+
+    llvm::StringRef from = in_string.take_front(idx);
+    llvm::StringRef to = right1.drop_front(specifier_str.size());
 
-  if (!is_range_expression) {
-    *range_start_len = 0;
-    *range_end_pos = 0;
+    if (BreakpointID::IsValidIDExpression(from) &&
+        BreakpointID::IsValidIDExpression(to)) {
+      return std::make_pair(from, to);
+    }
   }
 
-  return is_range_expression;
+  return std::pair<llvm::StringRef, llvm::StringRef>();
 }