Re: [rfa] linespec.c, part 5

[Elena Zannoni <ezannoni at redhat dot com>]

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