From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9814 invoked by alias); 28 Mar 2012 20:46:21 -0000 Received: (qmail 9760 invoked by uid 22791); 28 Mar 2012 20:46:18 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,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; Wed, 28 Mar 2012 20:46:04 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q2SKk3gG005231 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 28 Mar 2012 16:46:03 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q2SKk2O9008421 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 28 Mar 2012 16:46:02 -0400 From: Tom Tromey To: Keith Seitz Cc: "gdb-patches\@sourceware.org ml" Subject: Re: [RFA 1/2] Linespec rewrite (update 2) References: <4F70F8F7.503@redhat.com> Date: Wed, 28 Mar 2012 20:46:00 -0000 In-Reply-To: <4F70F8F7.503@redhat.com> (Keith Seitz's message of "Mon, 26 Mar 2012 16:17:11 -0700") Message-ID: <87zkb0wj1x.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.94 (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-03/txt/msg00963.txt.bz2 >>>>> "Keith" == Keith Seitz writes: Keith> Tom requested a repost of this patch, so here it is. Thanks, Keith. This patch is pretty hard to review. I've been reading the branch instead, but even there I find it hard to wrap my head around it all. That said, I like what I see. Congratulations on conquering linespec. I'm glad to see others testing this patch on their various scenarios, I think that raises the confidence level in the patch. Keith> +const char * const linespec_keywords[] = { "if", "thread", "task" }; Should also be static. Keith> + /* If we're in list mode, and the next token is a string beginning Keith> + with ",", we're dealing with a ranged listing. Stop parsing Keith> + and return. */ Keith> + if (PARSER_STATE (parser)->list_mode Keith> + && token.type == LSTOKEN_STRING Keith> + && *LS_TOKEN_STOKEN (token).ptr == ',') Keith> + return; I thought that historically a top-level comma always terminated a linespec -- not just in list mode. It should be possible to write a test case for this in Python pretty easily. Keith> +static void Keith> +canonicalize_linespec (struct linespec_state *state, linespec_t ls) [...] Keith> + if (ls->line_offset.sign != unknown) Keith> + { Keith> + if (need_colon) Keith> + fputc_unfiltered (':', buf); Keith> + fprintf_filtered (buf, "%s%d", Keith> + (ls->line_offset.sign == none ? "" Keith> + : ls->line_offset.sign == plus ? "+" : "-"), Keith> + ls->line_offset.offset); I am curious when this code can trigger. Can we end up with a canonical form like "function:+5"? I was hoping to reserve that syntax for a later addition; and anyway in general I think relative linespecs need to be made absolute by the canonicalization process, since otherwise re-setting won't do the right thing. Keith> + /* We have an expression. No other attribute is allowed. */ It would be helpful if the constraints on the fields of 'struct linespec' were documented there. Keith> + pspace = elem->minsym->ginfo.obj_section->objfile->pspace; Should use SYMBOL_OBJ_SECTION. I didn't audit for other instances. Keith> + else if (ls->minimal_symbols != NULL) Keith> + { Keith> + /* We found minimal symbols, but no normal symbols. */ Keith> + int i; Keith> + minsym_and_objfile_d *elem; Keith> + Keith> + for (i = 0; Keith> + VEC_iterate (minsym_and_objfile_d, ls->minimal_symbols, i, elem); Keith> + ++i) Keith> + minsym_found (state, elem->objfile, elem->minsym, &sals); Why are minsyms sorted by pspace in one branch but not another? Keith> +++ b/gdb/testsuite/gdb.cp/cplabel.exp [...] Keith> +if {[prepare_for_testing "$testfile.exp" $testfile $srcfile]} { Keith> + return -1 I suspect this needs a skip_cplus_tests check. Tom