This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] change the arguments of create_breakpoint to a struct
[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