This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Have block_innermost_frame start from selected frame
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Paul Hilfinger <Hilfinger at adacore dot com>
- Cc: Eli Zaretskii <eliz at gnu dot org>, gdb-patches at sourceware dot org
- Date: Tue, 10 Jan 2012 08:32:13 +0400
- Subject: Re: [RFA] Have block_innermost_frame start from selected frame
- References: <20111230215222.86B713FEE8@kwai.gnat.com> <8339c1td3u.fsf@gnu.org> <20120109071645.93E9F92BF6@kwai.gnat.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