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

[Keith Seitz <keiths@redhat.com>]

On 04/03/2012 02:18 PM, Doug Evans wrote:

> 1)  Replace "_t" suffix with "p" or "_p" or some such.

Done.

> 2) [enums UPPERCASE & prefixed]
>    /* 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
> };
 > 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?

Done.

> 3) struct linespec
> 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.

I've added some comments. Please review and let me know if you think 
more is needed.

This also elided the fact that find_label_symbols requires its third 
parameter and it may not be NULL. I've removed the check for non-NULL 
LABEL_FUNCS_RET in that function (and checked all callers).

> 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.

Done.

> 6)
>
>        /* Special case: Ada operators.  */
>        if (current_language->la_language == language_ada
>            &&  quote_char == '\"')
>
> So some aspect of ' vs " remains?

Ada allows operators named (literally) "+". The quote is part of the 
actual method name. That's what the check is attempting to ascertain.

> 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).

Done. I had this in an earlier version. I don't know why I changed it.

> 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?

If the linespec contains a filename, it *must* also have LSTOKEN_COLON. 
That is the only permitted usage of a filename. Since this case is 
already dealt with by the optimization earlier (where it checks if the 
next token is LSTOKEN_COLON before looking for the symtab), this isn't 
really needed at all. I've removed it.

> 8)
>
> /* Initialize a new linespec parser.  */
> static void
> linespec_parser_new (linespec_parser *parser,
>
> Blank line after comment.

Added.

I've pushed the requested changes to my archer branch. [Tom's asked me 
to repost it as well.]

Thank you very much for taking a look.

Keith