Comments

On Wed, Nov 17, 2010 at 10:29:29AM -0500, Nathan Froyd wrote:
> This patch attempts to fix that by detecting when we've just parsed a> declaration and we're faced with what looks like the beginning of the> next declaration.
I forgot that one testcase needed some adjustments. The patch below
deletes some dg-error messages since for the opening line of the
testcase:
typedef BYTE unsigned char; /* { dg-error "expected" } */
we now treat BYTE as a type. I think this is an improvement, albeit an
unexpected one.
-Nathan

On 11/17/2010 04:33 PM, Nathan Froyd wrote:
> On Wed, Nov 17, 2010 at 10:29:29AM -0500, Nathan Froyd wrote:>> This patch attempts to fix that by detecting when we've just parsed a>> declaration and we're faced with what looks like the beginning of the>> next declaration.>> I forgot that one testcase needed some adjustments. The patch below> deletes some dg-error messages since for the opening line of the> testcase:>> typedef BYTE unsigned char; /* { dg-error "expected" } */>> we now treat BYTE as a type. I think this is an improvement, albeit an> unexpected one.
Ah, so we parse it as
typedef /* implied int */ BYTE;
unsigned char;
though without the extra warnings the above has. This may be an
improvement (better parsing recovery is good, but better semantic
recovery is too!), but I'm not sure about the relatively convoluted way
in which the parser gets there.
In particular, I'm worried that for example in something like this:
typedef uintt16_t pid_t;
we would not be able to see the declaration of pid_t as a type. In
other words, I'm worried that it becomes harder to improve this tescase:
typedef unsigned short uint16_t;
/* should warn about unknown type name "uintt16_t", currently gives
a parse error ("expected ... before pid_t") because unknown type
names are not guessed in c_parser_declspecs. */
typedef uintt16_t pid_t;
/* no error expected about unknown type name; currently fails */
pid_t xyz;
Given my recent experience with adding unknown type name errors, it
should not be hard to fix the above (just lack of time on my part), for
example by moving this into c_parser_next_token_starts_typename:
/* Try a bit harder to detect an unknown typename. */
if (token->type == CPP_NAME
&& token->id_kind == C_ID_ID
&& (c_parser_peek_2nd_token (parser)->type == CPP_NAME
|| c_parser_peek_2nd_token (parser)->type == CPP_MULT)
&& !lookup_name (token->value)
/* Do not try too hard when we could have "object in array". */
&& !parser->objc_could_be_foreach_context)
return true;
Maybe you can give this a shot before applying these patches?
Paolo

On 11/18/2010 08:04 PM, Nathan Froyd wrote:
> I think> the saving grace on your testcase is that in:>> typedef uintt16_t pid_t;>> `pid_t' is not a keyword, so we don't stop parsing early, but I haven't> traced through the parser to verify that.
Yes, that's possible. In that case, I have no objection to the patch;
the testcase is quite weird indeed in having a keyword after the id.
For what is worth, my WIP patch has no effect on your testcase:
$ ./cc1
typedef BYTE unsigned char;
BYTE x;
<stdin>:1:14: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’
before ‘unsigned’
<stdin>:2:1: error: unknown type name ‘BYTE’
but fixes the other:
$ ./cc1
typedef uintt16_t pid_t;
pid_t x;
<stdin>:1:1: error: unknown type name ‘uintt16_t’
Thanks,
Paolo