Re: [PATCH] change the arguments of create_breakpoint to a struct

[Keith Seitz <keiths@redhat.com>]

[I'm replying to Sergio's response because he makes a few points upon 
which I would like to expound. I've also trimmed everyone but Pedro, who 
is on holiday this week, since I think he has touched this code more 
than just about anybody. His input is definitely worth waiting for.]

On 02/21/2013 05:58 AM, Sergio Durigan Junior wrote:
> On Thursday, February 21 2013, Hui Zhu wrote:
>
>> According to the discussion in
>> http://sourceware.org/ml/gdb/2012-08/msg00001.html
>>
>> I post a new patch for GDB to change the arguments of
>> create_breakpoint to a struct.

In general, I think this patch has merit. There has been a lot of 
"cruft" added to create_breakpoint over the years, and it is an 
undeniable mess of options. I've been playing in breakpoint.c a lot over 
the past few months (well, it seems like decades!) to feel brave enough 
to offer an opinion here. Call breakpoint.c my new linespec.c (almost).

IMO the real problem with create_breakpoint isn't necessarily that there 
are a bunch of options tacked onto the argument list. The real problem 
(again *for me*/IMO) is that a lot of these options aren't logically 
grouped together.

This does need cleaning up, as you rightly point out, and I am glad that 
you are pursuing this.

While names of struct members naturally tends to explain what the 
various bits mean -- a deficiency in the current API -- I think that 
this patch both goes too far with collecting options together and then 
not far enough in propagating this API change internally.

Let's look at a typical use of create_breakpoint, most of which look a 
lot like:

   create_breakpoint (gdbarch,
		     linespec, cond_string, -1, NULL,
		     0 /* condition and thread are valid.  */,
		     tempflag, bp_breakpoint,
		     0,
		     AUTO_BOOLEAN_TRUE /* pending */,
		     &breakpoint_ops, from_tty,
		     1 /* enabled */,
		     0 /* internal */,
		     0);

Your patch would address the question, "What are -1, NULL, 0, and 0?" [I 
have to look those up -- I haven't memorized the API.

-1 = thread (-1 meaning "all threads")
NULL = extra_string
0 = ignore_count
0 = flags

Shame on me?]

I think this boils down to figuring out what is important information to 
convey to the reader of the code. What are the logical portions of the 
arguments for this API function that make sense to group together?

Condition string, thread, extra string, ignore count, enabled, and 
internal are all (mutable) properties of a breakpoint -- /any/ breakpoint.

You can use this basic set of (optional) properties to create any 
breakpoint type (with some exceptions, of course). from_tty, flags, 
tempflag (why isn't that in flags?), type_wanted, ops, arch, and 
linespec describe other important information that are not mutable. Some 
are more flags for describing how the breakpoint should be created as 
opposed to what the breakpoint is and how it should behave.

So, I would rather see create_breakpoint look more (perhaps not exactly, 
though) like:

   create_breakpoint (gdbarch, type_wanted, ops, linespec, \
       new_optional_breakpoint_properties_struct, from_tty, \
       create_flags)

Here, I've added tempflag, parse_condition_and_thread, and pending, to 
create_flags. I would probably even go a bit further and attempt to roll 
from_tty into create_flags, since that describes the behavior of 
create_breakpoint.

I would also try to tie type_wanted and ops together in some way, too, 
if I could. I don't actually know if that can be done. I don't know if 
there is a 1:1 mapping on type and breakpoint type.

Assuming we could do all these things, create_breakpoint would look 
something more like:

     create_breakpoint (gdbarch, type_wanted, linespec, \
         new_optional_breakpoint_properties_struct, create_flags)

To me, that is a much cleaner function, one with all the most important 
information listed in the arguments to the function and all the 
non-essential stuff hidden in a struct or flags.

But this is just one person's opinion, i.e., I am not a global maintainer.

>> --- a/breakpoint.c
>> +++ b/breakpoint.c
>> @@ -9493,32 +9493,27 @@ decode_static_tracepoint_spec (char **ar
>>     return sals;
>>   }
>>
>> +/* Initialize all the element of CB to its default value. */
>
> I think it is better to write "... to their default values."  Also
> missing double-space after period.

Or "/* Initialize all elements of CB to their default values.  */" :-) 
The curse of the native English speaker.

>> +void
>> +create_breakpoint_init (struct create_breakpoint_s *cb)
>> +{
>> +  memset (cb, '\0', sizeof (*cb));
>> +  cb->type_wanted = bp_breakpoint;
>> +  cb->pending_break_support = AUTO_BOOLEAN_TRUE;
>> +  cb->enabled = 1;
>> +}
>
> I would change the name of the fuction to `init_breakpoint_properties'
> or something (see more below).

Here Sergio seems to be already thinking along the lines of my earlier 
argument; we want a properties structure.

> I have read the discussion on the link you posted above, but still I
> want to confirm something: would it be possible to make this function
> take all the arguments that create_breakpoint et al now take?  My point
> is that, IMO, it would be clearer to pass all those arguments to this
> function directly, instead of filing them in the respective callers.
> Matter of taste, of couse.

I would go so far as to say that it would be nice, perhaps even 
desirable, to also pass this new properties struct down to 
create_breakpoints_sal, which also suffers from the same problem.

> Just to make my argument clearer, my suggestion is to actually replace
> this with:
>
>
>      init_breakpoint_properties (&cb, get_current_arch (), arg,
>                                  ... );
>      create_breakpoint (&cb);
>
> Don't know, it seems more organized to me.  But of course, you might
> just want to wait for some maintainer to give his suggestion :-).
>

That seems to be the way a lot of people, myself included, are writing 
initializers.

> I liked Stan's proposal, to name the structure "struct
> breakpoint_properties", so I suggest using this name.

Me, too! But I don't think every parameter to create_breakpoint is 
necessarily a "property" of the breakpoint. After the breakpoint is 
created, for example, its type is immutable (amongst other things). 
These types of properties I would prefer be separate from those which 
are mutable.

Well, I'm sure that's more than you really wanted to read about this 
patch. I wouldn't necessarily encourage you to do anything about which 
I've expounded without hearing from one of the global maintainers first, 
especially Pedro (who returns next week).

Thank you for listening/reading.

Keith