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

[Jim Ingham via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

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.



> BreakpointID.h:74-75
>    //------------------------------------------------------------------
> -  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);
>  

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

> BreakpointIDList.cpp:329-330
>  
> -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()) {

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

https://reviews.llvm.org/D25158