[Lldb-commits] [PATCH] D25158: Convert some Breakpoint to StringRef

[Jim Ingham via lldb-commits]

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