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

80–81

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

350–351

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

75–76

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

source/Breakpoint/BreakpointIDList.cpp

350–357

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

[zturner]

source/Breakpoint/BreakpointIDList.cpp

350–357

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/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,
+  static bool ParseCanonicalReference(llvm::StringRef input,
                                       lldb::break_id_t *break_id,
                                       lldb::break_id_t *break_loc_id);
 
   //------------------------------------------------------------------
   /// 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.
   /// @param[out] error

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"
 
public:
 
   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);
+  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) {
+bool BreakpointID::IsValidIDExpression(llvm::StringRef str) {
   break_id_t bp_id;
   break_id_t loc_id;
   BreakpointID::ParseCanonicalReference(str, &bp_id, &loc_id);
 
   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,
+bool BreakpointID::ParseCanonicalReference(llvm::StringRef 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;
 
-  if (input == nullptr || *input == '\0')
+  if (input.empty())
     return false;
+  break_id_t id = LLDB_INVALID_BREAK_ID;
+  break_id_t loc_id = LLDB_INVALID_BREAK_ID;
 
-  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, id))
+    return false;
 
-  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 false;
+  }
 
-  // Badly formatted canonical reference.
-  *break_id_ptr = LLDB_INVALID_BREAK_ID;
-  *break_loc_id_ptr = LLDB_INVALID_BREAK_ID;
+  // And at the end, the entire string must have been consumed.
+  if (!input.empty())
     return false;
+
+  *break_id_ptr = id;
+  *break_loc_id_ptr = loc_id;
+  return true;
 }
 
 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

 //  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)) {
+      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)) {
           break_id_t bp_id;
           break_id_t bp_loc_id;
 
-          BreakpointID::ParseCanonicalReference(bp_id_str.c_str(), &bp_id,
-                                                &bp_loc_id);
+          BreakpointID::ParseCanonicalReference(bp_id_str, &bp_id, &bp_loc_id);
           BreakpointSP breakpoint_sp = target->GetBreakpointByID(bp_id);
           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());
             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;
+    if (!is_range) {
+      new_args.AppendArgument(current_arg);
+      continue;
+    }
+
 
-      BreakpointID::ParseCanonicalReference(range_start.c_str(), &start_bp_id,
-                                            &start_loc_id);
-      BreakpointID::ParseCanonicalReference(range_end, &end_bp_id, &end_loc_id);
+    BreakpointID::ParseCanonicalReference(range_from, &start_bp_id,
+    BreakpointID::ParseCanonicalReference(range_to, &end_bp_id, &end_loc_id);
 
-      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;
-      }
+                                   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;
-      }
+                                   range_to.str().c_str());
 
-      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;
-      }
 
-      // 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>();
 }