From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29148 invoked by alias); 13 Jan 2012 17:09:34 -0000 Received: (qmail 29140 invoked by uid 22791); 13 Jan 2012 17:09:32 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 13 Jan 2012 17:09:16 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q0DH9GvJ002269 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 13 Jan 2012 12:09:16 -0500 Received: from psique ([10.3.112.11]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q0DH9Aot018344; Fri, 13 Jan 2012 12:09:12 -0500 From: Sergio Durigan Junior To: Pedro Alves Cc: gdb-patches@sourceware.org, Tom Tromey Subject: Re: [RFC] Make static tracepoint with markers more OO References: <4F1010F5.4020104@redhat.com> X-URL: http://www.redhat.com Date: Fri, 13 Jan 2012 17:24:00 -0000 In-Reply-To: <4F1010F5.4020104@redhat.com> (Pedro Alves's message of "Fri, 13 Jan 2012 11:09:41 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes 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/msg00487.txt.bz2 On Friday, January 13 2012, Pedro Alves wrote: > On 01/13/2012 04:01 AM, Sergio Durigan Junior wrote: >> -/* Assuming we're creating a static tracepoint, does S look like a >> - static tracepoint marker spec ("-m MARKER_ID")? */ >> -#define is_marker_spec(s) \ >> - (s != NULL && strncmp (s, "-m", 2) == 0 && ((s)[2] == ' ' || (s)[2] == '\t')) >> +/* Return 1 if B refers to a static tracepoint marker, zero otherwise. */ > > I think > > /* Return 1 if B refers to a static tracepoint set by marker ("-m"), zero otherwise. */ > > would be clearer. Thanks, will fix it. >> +static int strace_marker_p (struct breakpoint *b); > > >> - if (b->type == bp_static_tracepoint && !marker_spec) >> + if (strace_marker_p (b)) > > This one looks wrong. The old condition had a `!', so this was for > static tracepoints _not_ set through a marker. Ops, you're right, I missed that. >> + >> + /* Create SALs from address string, storing the result in linespec_result. >> + Return 1 on success, or zero otherwise. >> + >> + For an explanation about the arguments, see the function >> + `create_sals_from_address_default'. >> + >> + This function is called inside `create_breakpoint'. */ >> + int (*create_sals_from_address) (char **, struct linespec_result *, >> + enum bptype, int *, char *, char **); >> + >> + /* This method will be responsible for creating a breakpoint given its SALs. >> + Usually, it just calls `create_breakpoints_sal' (for ordinary >> + breakpoints). However, there may be some special cases where we might >> + need to do some tweaks, e.g., see >> + `strace_marker_init_or_create_breakpoint_sal'. >> + >> + This function is called inside `create_breakpoint'. */ >> + void (*create_breakpoints_sal) (struct gdbarch *, >> + struct linespec_result *, >> + struct linespec_sals *, char *, >> + enum bptype, enum bpdisp, int, int, >> + int, const struct breakpoint_ops *, >> + int, int, int); > > It's unfortunate to be calling the breakpoint's virtual methods > before the object itself is created, which will require some redesign > and refactoring if we ever switch to C++ (and is dangerous, as you may > end up touching parts of the object which are not constructed yet by > mistake), but, this is no worse than what we have now, so I'm fine with it. Yes, I understand what you're saying. I couldn't figure out a better way of handling this (except creating a "pre_breakpoint_ops"?). Anyway, thanks for the review, I will submit a fixed version of the patch in Tromey's reply.