Re: [RFA 1/2] Linespec rewrite (update 2)

[Doug Evans <dje@google.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.