[RFA] Have block_innermost_frame start from selected frame
Joel Brobecker
brobecker@adacore.com
Tue Jan 10 05:21:00 GMT 2012
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
More information about the Gdb-patches
mailing list