jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land.
This looks correct to me. I had a couple of trivial inline comments, but this looks fine. > BreakpointID.cpp:80-81 > > -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) { 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... > BreakpointIDList.cpp:350-351 > > -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()) { 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? https://reviews.llvm.org/D25158 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits