From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29739 invoked by alias); 3 Apr 2012 21:19:14 -0000 Received: (qmail 29731 invoked by uid 22791); 3 Apr 2012 21:19:13 -0000 X-SWARE-Spam-Status: No, hits=-5.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-vx0-f169.google.com (HELO mail-vx0-f169.google.com) (209.85.220.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 03 Apr 2012 21:18:56 +0000 Received: by vcbfk14 with SMTP id fk14so134759vcb.0 for ; Tue, 03 Apr 2012 14:18:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding:x-system-of-record :x-gm-message-state; bh=1usqy4qw36TqF8iGxz92tbiAcGXWVq8QNyHZzr0PWro=; b=GNTXKLLqlsmlR3VXc+T2dsX1PIYEpkSTgYU8HplRcz1Ku6+SQyflqZomZWvExcRpjT fe16S3HlrDm68kWk6P5nyuDV55zFO4W2cG/gxJQWPLTZT3Z7x2UQTUdo53vEid+s6E7b CSPuzzZtV4nsaeuvtYSTvwBuDKSMiSE8exAlBSF3MvthAF79DD+NSuv2W9NtvHO81duR FYXwxQDaWTNYmCcUWIrTerB9GlTGNbR5ggq/lZlSaDC41h+qGMdxUtWRvj7OubdehvBZ m82tKulfLQ/G5RVi88oBJEeuSl36axhhRUZ28dUzX5L5sNu7MCn+lYZNQ60TcDVa+8XF uc4Q== Received: by 10.220.188.12 with SMTP id cy12mr6383582vcb.69.1333487935457; Tue, 03 Apr 2012 14:18:55 -0700 (PDT) MIME-Version: 1.0 Received: by 10.220.188.12 with SMTP id cy12mr6383575vcb.69.1333487935357; Tue, 03 Apr 2012 14:18:55 -0700 (PDT) Received: by 10.220.73.14 with HTTP; Tue, 3 Apr 2012 14:18:55 -0700 (PDT) In-Reply-To: <4F70F8F7.503@redhat.com> References: <4F70F8F7.503@redhat.com> Date: Tue, 03 Apr 2012 21:19:00 -0000 Message-ID: Subject: Re: [RFA 1/2] Linespec rewrite (update 2) From: Doug Evans To: Keith Seitz Cc: "gdb-patches@sourceware.org ml" Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-System-Of-Record: true X-Gm-Message-State: ALoCoQm30eTbaY3gjDDuDe/kbWzyxIG119p1YW/JAeI8if2Uo5EK63xOL0dYEE8cKk59k+KoCObrZ+Hq7szpsLx7N52wb0sGJMUjny0W4E0utOcnPqilEknZzyLrPD+osNud6vZkAsH1auDqJgRUGGXOcfgUQXlzTw== 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-04/txt/msg00048.txt.bz2 On Mon, Mar 26, 2012 at 4:17 PM, Keith Seitz wrote: > Hi, > > Tom requested a repost of this patch, so here it is. Here is a list of > differences between this version and the previously posted patch: > > * skip_quote_char rewritten (Doug found a host of bugs in it) > * added backward compatibility for quote-enclosed linespecs (so no NEWS > entry needed) > * copy_token_string now uses replace_trailing_whitespace function from > cli/cli-utils.[ch] > * miscellaneous test fiddling (from other posts by Tom and Pedro) > * some new error checking/conditions added (bugs reported by Doug) > > This patch is now essentially standalone, as Pedro requested. [Previously= I > posted patches to be committed all together when finally approved.] > > Keith > > ChangeLog > 2012-03-26 =A0Keith Seitz =A0 Hi. I don't know if any of these have been addressed, but here are my new comme= nts. All nits. 1) +typedef struct linespec *linespec_t; Replace "_t" suffix with "p" or "_p" or some such. E.g., typedef struct symtab *symtab_p; typedef struct symbol *symbolp; typedef struct type *typep; 2) /* An enumeration of possible signs for a line offset. */ enum offset_relative_sign { /* No sign */ none, /* A plus sign ("+") */ plus, /* A minus sign ("-") */ minus, /* A special "sign" for unspecified offset. */ unknown }; --> /* An enumeration of possible signs for a line offset. */ enum line_offset_sign { /* No sign */ line_offset_none, /* A plus sign ("+") */ line_offset_plus, /* A minus sign ("-") */ line_offset_minus, /* A special "sign" for unspecified offset. */ line_offset_unknown }; 3) In: /* A linespec. Elements of this structure are filled in by a parser (either parse_linespec or some other function). The structure is then converted into SALs by convert_linespec_to_sals. */ struct linespec { /* An expression and the resulting PC. */ char *expression; CORE_ADDR expr_pc; /* Any specified file symtabs. */ char *source_filename; VEC (symtab_p) *file_symtabs; /* The name of a function or method and any matching symbols. */ char *function_name; VEC (symbolp) *function_symbols; VEC (minsym_and_objfile_d) *minimal_symbols; /* The name of a label and matching symbols. */ char *label_name; struct { VEC (symbolp) *label_symbols; VEC (symbolp) *function_symbols; } labels; /* Line offset */ struct line_offset line_offset; }; The members here need more documentation. Something like explicitly stating that if the value was absent in the linespec, then the pointer is NULL and the corresponding values (e.g., expr= _pc) are invalid. 4) /* Token types */ enum ls_token_type { /* A keyword */ LSTOKEN_KEYWORD =3D 0, Yay, uppercase enum values. Can we uppercase all enum values in this file? 5) static char * skip_quote_char (const char *string, char quote_char) Return const char *, and if the cast to char * is necessary, put it in the caller. 6) /* Special case: Ada operators. */ if (current_language->la_language =3D=3D language_ada && quote_char =3D=3D '\"') So some aspect of ' vs " remains? Plus, IWBN if the language to use was not taken from a global. It can start out as a global, but the innards should take it as a parameter (e.g. record in linespec_parser). 7) In parse_linespec: /* This token must be LSTOKEN_COLON. */ if (token.type !=3D LSTOKEN_COLON) return values; It's not clear to me what case this is handling. Is "must" too strong a word? When will the "then" clause be taken? 8) /* Initialize a new linespec parser. */ static void linespec_parser_new (linespec_parser *parser, Blank line after comment.