From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6541 invoked by alias); 28 Mar 2011 18:46:58 -0000 Received: (qmail 6497 invoked by uid 22791); 28 Mar 2011 18:46:56 -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; Mon, 28 Mar 2011 18:46:52 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 7999F2BB00B; Mon, 28 Mar 2011 14:46:51 -0400 (EDT) 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 hwZ5PQJrfvuq; Mon, 28 Mar 2011 14:46:51 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 4BA992BAFCC; Mon, 28 Mar 2011 14:46:51 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 5E3D0145869; Mon, 28 Mar 2011 11:46:47 -0700 (PDT) Date: Mon, 28 Mar 2011 20:08:00 -0000 From: Joel Brobecker To: Thiago Jung Bauermann Cc: gdb-patches ml Subject: Re: [RFA] Refactor breakpoint_re_set_one Message-ID: <20110328184647.GD3670@adacore.com> References: <1301322874.2433.0.camel@hactar> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1301322874.2433.0.camel@hactar> 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: 2011-03/txt/msg01137.txt.bz2 > 2011-03-27 Thiago Jung Bauermann > > * breakpoint.c (breakpoint_re_set_one): Factor out breakpoint-resetting > code from here ... > (reset_breakpoint): ... to here ... > (addr_string_to_sals): ... and here. I didn't really verify line by line that you just extracted out the code without actually changing something, but I think we can trust your copy/paste tool :-). No real issue on my end of things (just a few little nits). But I'm no longer a specialist of this area, so I'd wait a little to see if anyone else might have some comments (say, until the end of the week?). My comments below: > +/* Find the SaL locations corresponding to the given addr_string. */ By convention `addr_string' should be capitalized. It's one of these things I really wonder why we do it, but anyways... > + /* For pending breakpoints, it's expected that parsing will > + fail until the right shared library is loaded. User has > + already told to create pending breakpoints and don't need ^^^^ doesn't > + extra messages. If breakpoint is in bp_shlib_disabled > + state, then user already saw the message about that > + breakpoint being disabled, and don't want to see more ^^^^^ doesn't > + errors. */ > +/* Reset a hardware or software breakpoint. */ > + > +static void > +reset_breakpoint (struct breakpoint *b) My only comment is that `reset' is a little ambiguous, but maybe that's just me. I often think of "reset" as set back to original value. I like the use of `re_set' in breakpoint_re_set_one, so what do you think of doing the same here? Also, a more descriptive description of the function would be useful, IMO. -- Joel