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: Keith Seitz <keiths at redhat dot com>
- To: Doug Evans <dje at google dot com>
- Cc: "gdb-patches at sourceware dot org ml" <gdb-patches at sourceware dot org>
- Date: Tue, 03 Apr 2012 16:14:14 -0700
- Subject: Re: [RFA 1/2] Linespec rewrite (update 2)
- References: <4F70F8F7.503@redhat.com> <CADPb22QsGLV71YwD-T2U+m=D4YWg1MXcAs5AmDi2xfU4XLwnzw@mail.gmail.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