On 08/02/2014 04:38 PM, Doug Evans wrote:
>   > 	(event_location_empty_p): Handel probe locations.
>
> "Handle ...", or just "Likewise."

Fixed.

>   > +    case PROBE_LOCATION:
>   > +      /* Probes are handled by their own decoders.  */
>   > +       gdb_assert_not_reached ("attempt to decode probe location");
>
> extra space of indentation

Fixed.

>   > +      if (cs != NULL && probe_linespec_to_ops (&cs) != NULL)
>   > +	{
>   > +	  location = new_probe_location (*stringp);
>   > +	  if (*stringp != NULL)
>
> This test is unnecessary (right?).

Yes, that's correct. I've removed it.

> btw, when will this function be passed *stringp == NULL?
> Can this function have an "early exit" if *stringp == NULL?
> [assuming that makes sense]

NULL is a valid linespec, e.g, a user does:
(gdb) start
(gdb) break
Breakpoint 1, ...

As to an early exit, ... I didn't implement it. If you would like me to, 
just say the word!

>   > @@ -173,12 +174,12 @@ parse_probes (struct event_location *location,
>   >      {
>   >        canonical->special_display = 1;
>   >        canonical->pre_expanded = 1;
>   > -      canonical->location = new_linespec_location (NULL);
>   > -      EVENT_LOCATION_LINESPEC (canonical->location)
>   > +      canonical->location = new_probe_location (NULL);
>   > +      EVENT_LOCATION_PROBE (canonical->location)
>   >  	= savestring (arg_start, arg_end - arg_start);
>
> I see why you pass NULL to new_probe_location and then set EVENT_LOCATION_PROBE
> afterwards (otherwise two copies of the string would be malloc'd, and you'd
> need to deal with freeing one of them).  One could add a version of
> new_probe_location that took a char* and a length, but that seems excessive.
> Another place where c++ would allow cleaner code.

As mentioned in patch #4, I've fixed in the prescribed manner (free the 
savestring after calling the ctor).

>
>   >      }
>   >
>   > -  EVENT_LOCATION_LINESPEC (location) = arg_end;
>   > +  EVENT_LOCATION_PROBE (location) = arg_end;
>   >    do_cleanups (cleanup);
>
> The function starts out with:
>
>    arg_start = EVENT_LOCATION_PROBE (location);
>
> and at the end we do this:
>
>    EVENT_LOCATION_PROBE (location) = arg_end;
>
> IWBN to document why we do this, it's not obvious to me why that is.
> [doesn't have to be part of this patch set though]

This now uses the more explicit API functions to do this, 
event_location_advance_ptr and added a comment. [Of course, this is part 
of the "skip past any processed input" paradigm that we have to deal 
with for linespecs/probes.]

>
>   >
>   >    return result;
>

Updated patch attached.

Keith

Changes since last revision:
   - Fix "Handel" in ChangeLog
   - Remove leading whitespace
   - Remove unnecessary NULL check in string_to_event_location
   - Pass savestring() to new_probe_location instead of
     NULL and accessing struct directly
   - Use functions instead of EVENT_LOCAITON_* macros.
   - Add comment and use event_location_advance_ptr to "skip"
     past processed input in parse_probes