From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30945 invoked by alias); 17 Aug 2012 19:00:33 -0000 Received: (qmail 30933 invoked by uid 22791); 17 Aug 2012 19:00:32 -0000 X-SWARE-Spam-Status: No, hits=-6.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_RG 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, 17 Aug 2012 19:00:14 +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 q7HJ09t2025005 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 17 Aug 2012 15:00:14 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q7HJ03jo018362 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 17 Aug 2012 15:00:08 -0400 From: Tom Tromey To: Keith Seitz Cc: "gdb-patches\@sourceware.org ml" Subject: Re: [RFA 3/5] Explicit linespecs - implementation References: <50120F6C.2000308@redhat.com> Date: Fri, 17 Aug 2012 19:00:00 -0000 In-Reply-To: <50120F6C.2000308@redhat.com> (Keith Seitz's message of "Thu, 26 Jul 2012 20:47:56 -0700") Message-ID: <87d32pjqdo.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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-08/txt/msg00492.txt.bz2 >>>>> "Keith" == Keith Seitz writes: Keith> Ok, so here's what everyone has been waiting for. This patch adds the Keith> implementation of explicit linespecs, including CLI and MI support. Thanks, Keith. I'm very encouraged by this project. Keith> +/* Throw an exception for unsupported explicit linespec. */ Keith> + Keith> +static void ATTRIBUTE_NORETURN Keith> +explicit_linespec_unsupported (enum bptype type) Keith> +{ Keith> + error (_("explicit linespec not supported for %s"), bptype_string (type)); I found it pretty hard to track through the code to find out under what circumstances this can really be called. Is it really an error for users? Or should it be an assertion? Keith> + char *where = b->addr_string; Keith> + struct cleanup *free_where = make_cleanup (null_cleanup, NULL); Keith> + Keith> + if (b->explicit != NULL) Keith> + { Keith> + where = explicit_linespec_to_string (b->explicit, b->addr_string); Keith> + make_cleanup (xfree, where); Keith> + } Keith> + Keith> + printf_filtered (_(" (%s) pending."), where); Keith> + do_cleanups (free_where); I think it is better for users to preserve what they typed, warts and all, and just regurgitate that. Once upon a time we did not do this for breakpoint conditions -- we printed from the expression object instead. This turned out to be quite confusing since you would enter: cond 5 p == 0x2168bf0 and 'info b' would print if p == 34996784 ... which is hard to recognize as the same thing, more difficult to search for, etc. I think this also means that if the user enters an explicit linespec, it will be shown as a normal linespec, which seems sub-optimal. Keith> +static char * Keith> +explicit_lex_one (char **inp) [...] Keith> + const char *end = skip_quote_char (start + 1, quote_char); IIRC, skip_quote_char has a funny quoting style -- there's no way to quote a quote. I think for new code we ought to adopt a better convention, like what buildargv does. Keith> + /* Get the argument string. */ Keith> + oarg = explicit_lex_one (argp); Keith> + make_cleanup (xfree, oarg); [...] Keith> + else if (strncmp (opt, "-address", len) == 0) Keith> + els->expression = oarg; This means that users will have to type: break -address "complicated expression (\"string\")" Note how the string has to be quoted. That seems not very user-friendly. It would be nicer to parse the expression after -address, but that has other complications -- for one thing, it means that -address would have to come last, since parsing expression is "funny". I think that is the better tradeoff though, because reordering the arguments is less painful than quoting expressions. Also I wonder whether completion works in a quoted expression... A nice follow-up patch would be to expose this to the Python breakpoint constructor, say via keyword arguments. Tom