Re: [RFC] Make static tracepoint with markers more OO

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Friday, January 13 2012, Pedro Alves wrote:

> On 01/13/2012 04:01 AM, Sergio Durigan Junior wrote:
>> -/* Assuming we're creating a static tracepoint, does S look like a
>> -   static tracepoint marker spec ("-m MARKER_ID")?  */
>> -#define is_marker_spec(s)						\
>> -  (s != NULL && strncmp (s, "-m", 2) == 0 && ((s)[2] == ' ' || (s)[2] == '\t'))
>> +/* Return 1 if B refers to a static tracepoint marker, zero otherwise.  */
>
> I think
>
>  /* Return 1 if B refers to a static tracepoint set by marker ("-m"), zero otherwise.  */
>
> would be clearer.

Thanks, will fix it.

>> +static int strace_marker_p (struct breakpoint *b);
>
>
>> -      if (b->type == bp_static_tracepoint && !marker_spec)
>> +      if (strace_marker_p (b))
>
> This one looks wrong.  The old condition had a `!', so this was for
> static tracepoints _not_ set through a marker.

Ops, you're right, I missed that.

>> +
>> +  /* Create SALs from address string, storing the result in linespec_result.
>> +     Return 1 on success, or zero otherwise.
>> +
>> +     For an explanation about the arguments, see the function
>> +     `create_sals_from_address_default'.
>> +
>> +     This function is called inside `create_breakpoint'.  */
>> +  int (*create_sals_from_address) (char **, struct linespec_result *,
>> +				   enum bptype, int *, char *, char **);
>> +
>> +  /* This method will be responsible for creating a breakpoint given its SALs.
>> +     Usually, it just calls `create_breakpoints_sal' (for ordinary
>> +     breakpoints).  However, there may be some special cases where we might
>> +     need to do some tweaks, e.g., see
>> +     `strace_marker_init_or_create_breakpoint_sal'.
>> +
>> +     This function is called inside `create_breakpoint'.  */
>> +  void (*create_breakpoints_sal) (struct gdbarch *,
>> +				  struct linespec_result *,
>> +				  struct linespec_sals *, char *,
>> +				  enum bptype, enum bpdisp, int, int,
>> +				  int, const struct breakpoint_ops *,
>> +				  int, int, int);
>
> It's unfortunate to be calling the breakpoint's virtual methods
> before the object itself is created, which will require some redesign
> and refactoring if we ever switch to C++ (and is dangerous, as you may
> end up touching parts of the object which are not constructed yet by
> mistake), but, this is no worse than what we have now, so I'm fine with it.

Yes, I understand what you're saying.  I couldn't figure out a better
way of handling this (except creating a "pre_breakpoint_ops"?).  Anyway,
thanks for the review, I will submit a fixed version of the patch in
Tromey's reply.