Re: [PING] Re: [PATCH] Fix problem handling colon in linespec, PR breakpoints/18303

[Doug Evans <dje@google.com>]

On Mon, Jan 25, 2016 at 9:26 AM, Don Breazeal <donb@codesourcery.com> wrote:
> On 1/12/2016 10:26 AM, Keith Seitz wrote:
>> On 01/11/2016 02:34 PM, Doug Evans wrote:
>>>  > -     a complete test, just cheap and documentary.  */
>>>  > -  if (strchr (name, '<') == NULL && strchr (name, '(') == NULL)
>>>  > -    gdb_assert (strchr (name, ':') == NULL);
>>>  > -
>>>
>>> Heya.
>>>
>>> The assert is intended to catch (some) violations of this
>>> (from the function comment):
>>>
>>>    NAME is guaranteed to not have any scope (no "::") in its name, though
>>>    if for example NAME is a template spec then "::" may appear in the
>>>    argument list.
>> [snip]
>>> On that I'm kinda ambivalent, but I like having the assert
>>> watch for the stated invariant.
>>>
>>> Thoughts?
>>
>> I missed that comment. [Well, I didn't even look at it. I'm so used to
>> seeing no/minimal comments for symbol searching functions that I seldom
>> even look for them. My bad.]
>>
>> That seems like a reasonable assertion, then, as long as it really does
>> test what it is supposed to. How about:
>>
>>   if (strchr (name, '<') == NULL && strchr (name, '(') == NULL)
>>     gdb_assert (strstr (name, "::") == NULL);
>>
>> Or something like that?
>>
>>>  > diff --git a/gdb/cp-support.c b/gdb/cp-support.c
>>>  > index df127c4..a71c6ad 100644
>>>  > --- a/gdb/cp-support.c
>>>  > +++ b/gdb/cp-support.c
>>>  > @@ -1037,8 +1037,13 @@ cp_find_first_component_aux (const char *name,
>>>  > int permissive)
>>>  >            return strlen (name);
>>>  >          }
>>>  >      case '\0':
>>>  > -    case ':':
>>>  >        return index;
>>>  > +    case ':':
>>>  > +      /* ':' marks a component iff the next character is also a ':'.
>>>  > +         Otherwise it is probably malformed input.  */
>>>  > +      if (name[index + 1] == ':')
>>>  > +        return index;
>>>  > +      break;
>>>
>>> What if name[index+2] is also ':'? :-)
>>>
>>
>> I don't think that matters at all. It isn't the scope operator in C++
>> unless it is *two* colons. Not just a single colon. [Note that I believe
>> we are going to have to deal with the general single-colon issue when
>> running this code with abitags, but that's a patch for some other time.
>> Or maybe this patch already mitigates that to a degree. I haven't
>> checked into it at all.]
>>
>> Keith
>>
>
> Hi Doug, any thoughts on earlier responses from Keith and me to your
> comments on this issue?
> Thanks
> --Don

Sorry, nothing more to add.
Keith's suggestion is fine by me.