Re: [RFA] Have block_innermost_frame start from selected frame

[Joel Brobecker <brobecker@adacore.com>]

Hi Paul,

> 2011-12-27  Paul Hilfinger  <hilfingr@adacore.com>
> 
>   * gdb/blockframe.c (block_innermost_frame): Start search from selected frame,
>     if present, or otherwise the current frame.
> 
>   * gdb/c-exp.y, gdb/m2-exp.y, gdb/objc-exp.y: Update innermost_block
> 
>   * gdb/doc/gdb.texinfo (Variables): Document use of :: for non-static
>     variables.

When you create an entry in gdb's ChangeLog file, can you remember
to remove the "gdb/" prefix? Also, the doc/ subdirectory has its own
ChangeLog file as well, so remember to remove the "gdb/doc/" prefix
in the filename.

> -/* Return the innermost stack frame executing inside of BLOCK, or NULL
> -   if there is no such frame.  If BLOCK is NULL, just return NULL.  */
> +/* Return the innermost stack frame that is executing inside of BLOCK and is
> + * at least as old as the selected frame. Return NULL if there is no
> + * such frame.  If BLOCK is NULL, just return NULL.  */

Minor issue with formatting: Can you remove the '*' at the start of
the second and third line?

> -  frame = get_current_frame ();
> +  frame = get_selected_frame_if_set ();
> +  if (frame == NULL)
> +    frame = get_current_frame ();

I really flip-flopped on this, between using get_selected_frame_if_set
or just get_selected_frame. In the end, to me, the real difference
between the two is the fact that the second could potentially *change*
the global state (selected frame, language). These could be considered
side-effects while I have a feeling that block_innermost_frame is
a function that should really be pure. So I agree with you that using
get_selected_frame_if_set was the right choice.

> +			  if (symbol_read_needs_frame (sym))
> +			    {
> +			      if (innermost_block == 0
> +				  || contained_in (block_found, 
> +						   innermost_block))
> +				innermost_block = block_found;
> +			    }

I know you copied this from elsewhere in the file, and that elsewhere
also has that extraneous trailing space (`|| contained_in (block_found, '),
but could you remove it from this new hunk?

> +@smallexample
> +void
> +foo (int a)
> +@{
> +  if (a < 10)
> +      bar (a);
> +  else
> +      process (a);    /* Stop here */
> +@}

Nit-picking: The formatting looks a little off for the if and else
blocks.  It would be nicer, I think, if we remained consistent the GDB
style...

Eli?

> +@smallexample
> +(@value{GDBP}) p a
> +(@value{GDBP}) p bar::a
> +(@value{GDBP}) up 2
> +(@value{GDBP}) p a
> +(@value{GDBP}) p bar::a
> +@end smallexample
> +
> +@noindent
> +will print the values @samp{10}, @samp{5}, @samp{5}, and @samp{0} in that
> +order.

My 2 cents: We could also embed the GDB transcript, rather than
having the input on one end, and the output on the other hand.
Not a request to change, just some thoughts...

-- 
Joel