From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16792 invoked by alias); 30 Mar 2009 15:34:08 -0000 Received: (qmail 15765 invoked by uid 22791); 30 Mar 2009 15:34:05 -0000 X-SWARE-Spam-Status: No, hits=-2.4 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, 30 Mar 2009 15:33:55 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 146702BBF56; Mon, 30 Mar 2009 11:33:54 -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 Gv9Swj9tNZF8; Mon, 30 Mar 2009 11:33:54 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id D77982BBF54; Mon, 30 Mar 2009 11:33:53 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id A2F88F5899; Mon, 30 Mar 2009 08:33:48 -0700 (PDT) Date: Mon, 30 Mar 2009 15:44:00 -0000 From: Joel Brobecker To: Stan Shebs Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Make tracepoints into breakpoints Message-ID: <20090330153348.GY9472@adacore.com> References: <49CD275B.1020003@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <49CD275B.1020003@codesourcery.com> User-Agent: Mutt/1.5.18 (2008-05-17) 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: 2009-03/txt/msg00673.txt.bz2 Hi Stan, I was scanning your patch a little, to see waht you had needed to do in order to do this transition. I noticed a few things - not sure how important they are, given how quickly I looked at it. But there you go. > *************** bpstat_what (bpstat bs) > *** 3324,3329 **** > --- 3358,3367 ---- > bs_class = bp_silent; > retval.call_dummy = 1; > break; > + > + case bp_tracepoint: > + /* (should never occur, complain about it) */ > + continue; > } > current_action = table[(int) bs_class][(int) current_action]; > } This hunk looks really strange... Out of place? The thing that caught my attention too was the comment that seem to imply that this is a temporary comment, almost a FIXME. Not sure if this is the case, but the complaint is not there (elsewhere maybe?). > + /* Remove a tracepoint (or all if no argument) */ Nit-picking on the missing period at the end of your sentence. I just happened to notice it, but I haven't really checked the rest. > + #if 0 /* names of commands not visible here, not clear if worth fixing */ > + if (cmd_cfunc_eq (cmd, while_stepping_pseudocommand)) > + indent = i2; > + else if (cmd_cfunc_eq (cmd, end_actions_pseudocommand)) > + indent = i1; > + #endif Do we really want to keep this #if 0? I'd personally rather have a comment. I'd also like to look at your patch again sometime soon, to figure out whether it'd make sense or not for tracepoints to have their own breakpoint_ops routines... To be continued :) -- Joel