From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27525 invoked by alias); 16 Nov 2010 08:07:34 -0000 Received: (qmail 27503 invoked by uid 22791); 16 Nov 2010 08:07:33 -0000 X-SWARE-Spam-Status: No, hits=-2.1 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, 16 Nov 2010 08:07:28 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 8B7912BABF4; Tue, 16 Nov 2010 03:07:26 -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 BJBNlJmvsOzF; Tue, 16 Nov 2010 03:07:26 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 37D672BABF0; Tue, 16 Nov 2010 03:07:26 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id B68B3145C0D; Tue, 16 Nov 2010 00:07:21 -0800 (PST) Date: Tue, 16 Nov 2010 08:07:00 -0000 From: Joel Brobecker To: Jan Kratochvil Cc: Thiago Jung Bauermann , Pedro Alves , gdb-patches@sourceware.org Subject: Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops Message-ID: <20101116080721.GC4434@adacore.com> References: <1282074071.2606.702.camel@hactar> <201010161843.43062.pedro@codesourcery.com> <1287534691.2686.17.camel@hactar> <20101116040625.GB19243@host0.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20101116040625.GB19243@host0.dyn.jankratochvil.net> 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: 2010-11/txt/msg00198.txt.bz2 > > -static void > > -insert_catch_fork (struct breakpoint *b) > > +static int > > +insert_catch_fork (struct bp_location *b) > > Such variables (across the whole patch) should be really renamed when > changing its type. How about doing such a rename as a patch on its own? I've spent a fair amount of time thinking about this patch, and I'd rather not have to review it again if the changes are only going to be minor and not affect behavior. I don't have any issue with blocking checkin of this patch until patches implementing your suggestions are approved, if that helps. I just would rather avoid having to re-review the same patch again. Knowing how git works, this shouldn't be very hard to do. (notice: IIRC, when I first looked at patch #2, one of my reactions is that I wanted to see the patch split-up in several smaller pieces; I will explain that when I get to that patch) > Also were these functions intended per-breakpoint or per-bp_location? > It looks to me currently they are used only for single-location > breakpoint so no one knows. (I guess they were meant for breakpoint.) I think that eventually, we want them to be per bp-location. It does not matter right now, as you say, since they are only used for single- location breakpoints. That's a good point, though. Perhaps a documentation update can help make that clearer. OK with a separate patch? > > - void (*insert) (struct breakpoint *); > > + /* Insert the breakpoint or watchpoint or activate the catchpoint. > > + Return 0 for success, 1 if the breakpoint, watchpoint or catchpoint > > + type is not supported, -1 for failure. */ > > + int (*insert) (struct bp_location *); [...] > At least rename it to insert_bploc (or insert_location etc.). This > will need to be cleaned up with the regular breakpoints/watchpoints > conversion to breakpoint_ops. I don't feel that strongly about it. I don't feel that renaming "insert" that takes a bp_loc into "insert_bploc" is going to help much. But I'm OK. I am suggesting that we push that as a followup patch, if that's OK with you, just to ease review of that change alone. -- Joel