This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [rfa] linespec.c, part 5
- From: Elena Zannoni <ezannoni at redhat dot com>
- To: David Carlton <carlton at math dot stanford dot edu>
- Cc: Elena Zannoni <ezannoni at redhat dot com>, gdb-patches at sources dot redhat dot com
- Date: Thu, 5 Dec 2002 12:12:15 -0500
- Subject: Re: [rfa] linespec.c, part 5
- References: <ro1lm3uo4gt.fsf@jackfruit.Stanford.EDU><15829.39563.373999.459749@localhost.redhat.com><ro1adk95nrq.fsf@jackfruit.Stanford.EDU>
David Carlton writes:
> On Fri, 15 Nov 2002 20:08:27 -0500, Elena Zannoni <ezannoni@redhat.com> said:
> > David Carlton writes:
>
> >> * The function that this creates, decode_compound, is quite long.
> >> Later patches in this series will break it down into smaller
> >> chunks; they will come after I've got decode_line_1 itself down to
> >> a nice size.
>
> > OK but, could you add something to the comment at the top of the
> > function that describes what the function returns and what the
> > params are?
>
> Sure, I can elaborate on that comment. Though I don't think I want to
> describe _all_ of the args there: many functions get passed the args
> of decode_line_1, so I'd rather put a single comment describing that
> somewhere rather than repeating that over and over again.
>
> Maybe I'll replace the these comments right after the end of
> decode_line_1:
>
> /* Now, still more helper functions. */
>
> /* NOTE: carlton/2002-11-07: Some of these have non-obvious side
> effects. In particular, if a function is passed ARGPTR as an
> argument, it modifies what ARGPTR points to. (Typically, it
> advances *ARGPTR past whatever substring it has just looked
> at.) */
>
> with a comment saying:
>
> /* Now, more helper functions for decode_line_1. Some conventions
> that these functions follow:
>
> Decode_line_1 typically passes along some of its arguments or local
> variables to the subfunctions. It passes the variables by
> reference if they are modified by the subfunction, and by value
> otherwise.
>
> Some of the functions have side effects that don't arise from
> variables that are passed by reference. In particular, if a
> function is passed ARGPTR as an argument, it modifies what ARGPTR
> points to; typically, it advances *ARGPTR past whatever substring
> it has just looked at. (If it doesn't modify *ARGPTR, then the
> function gets passed *ARGPTR instead, which is then called ARG: see
> set_flags, for example.) Also, functions that return a struct
> symtabs_and_lines may modify CANONICAL, as in the description of
> decode_line_1.
>
> If a function returns a struct symtabs_and_lines, then that struct
> will immediately make its way up the call chain to be returned by
> decode_line_1. In particular, all of the functions decode_XXX
> calculate the appropriate struct symtabs_and_lines, under the
> assumption that their argument is of the form XXX. */
>
> Is that clearer?
>
better, yes.
> >> * Since decode_line_1 returns the value of decode_compound, there's no
> >> need to check whether or not decode_compound modifies the arguments
> >> that it's been passed: later code in decode_line_1 can never be
> >> effected by such modifications.
>
> > Hmmm. maybe I am not understanding what you say. Does
> > decode_compound modify its args? seems like it changes argptr and
> > canonical. What if somebody changes decode_line_1 in the future so
> > that there is more code executed after the call, it could probably
> > cause some head scratching to see that the call didn't leave things
> > as expected. I haven't looked too closely though.
>
> Does the above comment help? What I meant was: every code path in the
> code that I extracted leads to either a return from decode_line_1 or
> an error. So, for example, subsequent code in decode_line_1 won't
> depend on, say, modifications to 'p' within decode_compound.
>
> Decode_compound prepares ARGPTR and CANONICAL appropriately for
> return, and calculates the struct symtabs_and_lines to be returned.
> If somebody subsequently decides that, say, a bit more fiddling should
> happen within decode_line_1, I think these changes should make that
> fiddling a lot easier rather than a lot harder.
>
I guess we'll heve to wait for the rest of your patches. If that's
still an issue we can revisit later.
> It will also help once I've renamed some variables, so 's' turns into
> 'file_symtab', 'p' turns into 'next_component', and so forth. That
> will make it much easier for future readers of the code to understand
> the possible ramifications of changing the values of those variables.
> (Whereas who knows what it means to change the value of a variable 'p'
> that is referred to in a zillion places, with some of those uses
> distinct and some of them not.)
>
BTW, I did a diff -w of decode_compound and the code you removed, and
I noticed this (ignore the line numbers):
@@ -211,7 +231,6 @@
*argptr = (*p == '\'') ? p + 1 : p;
/* Look up entire name */
sym = lookup_symbol (copy, 0, VAR_NAMESPACE, 0, &sym_symtab);
- s = (struct symtab *) 0;
if (sym)
return symbol_found (funfirstline, canonical, copy, sym,
NULL, sym_symtab);
I agree with you that such assignment didn't make much sense. Dig dig dig,
it came in with the HP merge.
It was doing this:
/* Look up entire name */
sym = lookup_symbol (copy, 0, VAR_NAMESPACE, 0, &sym_symtab);
s = (struct symtab *) 0;
/* Prepare to jump: restore the " if (condition)" so outer layers see it */
if (has_if)
*ii = ' ';
/* Symbol was found --> jump to normal symbol processing.
Code following "symbol_found" expects "copy" to have the
symbol name, "sym" to have the symbol pointer, "s" to be
a specified file's symtab, and sym_symtab to be the symbol's
symtab. */
/* By jumping there we avoid falling through the FILE:LINE and
FILE:FUNC processing stuff below */
if (sym)
goto symbol_found;
and
symbol_found: was (is) checking for s==0.
But now that would be the NULL parameter in the symbol_found call
right after.
So I think that's safe.
OK check it in.
Elena