This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA 1/2] Linespec rewrite (update 2)
- From: Doug Evans <dje at google dot com>
- To: Keith Seitz <keiths at redhat dot com>
- Cc: "gdb-patches at sourceware dot org ml" <gdb-patches at sourceware dot org>
- Date: Tue, 3 Apr 2012 14:18:55 -0700
- Subject: Re: [RFA 1/2] Linespec rewrite (update 2)
- References: <4F70F8F7.503@redhat.com>
On Mon, Mar 26, 2012 at 4:17 PM, Keith Seitz <keiths@redhat.com> 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 ?Keith Seitz ?<keiths@redhat.com>
Hi.
I don't know if any of these have been addressed, but here are my new comments.
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 = 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 == language_ada
&& quote_char == '\"')
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 != 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.