[RFA 1/2] Linespec rewrite (update 2)
Keith Seitz
keiths@redhat.com
Tue Apr 3 23:14:00 GMT 2012
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
More information about the Gdb-patches
mailing list