On Tue, May 11, 2021 at 04:27:55PM +0200, Marcel Vollweiler wrote:
> > The usual wording would be
> > "too many %<always%> modifiers"
> >
>
> Changed for 'always' and 'close' for C and C++.
One extra thing, sorry, forgot to mention, for the translators it might be
better to use "too many %qs modifiers", "always" (or, "close").
That way they can translate it just once instead of twice.
> > IMHO you should at least check that tok->type == CPP_NAME before
> > checking pos + 1 token's type, you don't want to skip over CPP_EOF,
> > CPP_PRAGMA_EOF, or even CPP_CLOSE_PAREN etc.
> > Perhaps by adding
> > if (tok->type != CPP_NAME)
> > break;
> > right after c_token *tok = c_parser_peek_nth_token_raw (parser, pos); ?
>
> The check of the token's type at position 'pos' is done in the condition
> of the while loop, that means
> 'c_parser_peek_nth_token_raw (parser, pos + 1)->type == CPP_COLON'
> is only reached when
> 'c_parser_peek_nth_token_raw (parser, pos)->type == CPP_NAME'
> holds (since 'pos' is not changed in between).
You're right.
> > And, IMHO something should clear always and close (btw, might be better
> > to use close_modifier as variable name and for consistency always_modifier)
> > unless we reach the CPP_COLON case.
> >
>
> Good point, I agree with both. Cleared and renamed :)
I think the clearing is still insufficient.
It will clear on e.g. map (always, close, foobar)
but not on map (always) or map (always, close)
because in that case the loop terminates by the while loop condition
no longer being true.
And there is another thing I have missed (and again should be in the
testsuite):
map (always, always)
or
map (always, close, close)
etc. will with the patch diagnose that too many 'always' modifiers
(or 'close'), but that isn't correct diagnostic, there aren't any
modifiers, but the same variable is mapped multiple times.
So, one possibility is to remember details like:
potential always modifier has been seen
potential always modifier has been seen more than once
potential close modifier has been seen
potential close modifier has been seen more than once
and only when seeing the colon enact them and diagnose too many modifiers
(but then not with cp_parser_error but error with a location_t of one of the
modifiers), e.g. always_modifier == -1 would mean 1 potential has been seen,
== -2 more than one potential and == 1 it was modifier.
Or another one is not to do much in the first raw token lookup loop,
just check if it is a sequence of
always
close
,
tokens followed by
CPP_NAME (other than always, close) + CPP_CLONE combo
and in that case just set a bool flag that map-kind is present,
but don't consume any tokens.
And then in another loop if that bool flag is set, lookup non-raw
tokens and parse them, setting flags, doing diagnostics etc.
Basically do the look-ahead only to check if it is
map (var1, var2, ...)
case
or
map (modifiers map-kind: var1, var2, ...)
case.
Jakub
