From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 778 invoked by alias); 10 Jan 2012 04:32:50 -0000 Received: (qmail 766 invoked by uid 22791); 10 Jan 2012 04:32:47 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 10 Jan 2012 04:32:35 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 6C7B72BB111; Mon, 9 Jan 2012 23:32:34 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id GdIREdn-M7Np; Mon, 9 Jan 2012 23:32:34 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id CED422BB10E; Mon, 9 Jan 2012 23:32:33 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 6556E145615; Tue, 10 Jan 2012 08:32:13 +0400 (RET) Date: Tue, 10 Jan 2012 05:21:00 -0000 From: Joel Brobecker To: Paul Hilfinger Cc: Eli Zaretskii , gdb-patches@sourceware.org Subject: Re: [RFA] Have block_innermost_frame start from selected frame Message-ID: <20120110043213.GC31383@adacore.com> References: <20111230215222.86B713FEE8@kwai.gnat.com> <8339c1td3u.fsf@gnu.org> <20120109071645.93E9F92BF6@kwai.gnat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120109071645.93E9F92BF6@kwai.gnat.com> User-Agent: Mutt/1.5.20 (2009-06-14) Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-01/txt/msg00287.txt.bz2 Hi Paul, > 2011-12-27 Paul Hilfinger > > * 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