RFR: 8267580: The method JavacParser#peekToken is wrong when the first parameter is not zero

[Guoxiong Li]

On Mon, 24 May 2021 09:55:09 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Hi all,
>> 
>> The method `JavacParser#peekToken(int lookahead, Predicate<TokenKind>... kinds)` succeeds only when the first parameter `lookahead` is zero. If the `lookahead` is not zero, it is wrong and returns unexpected result.
>> 
>> The parameter `kinds` should not be dereferenced by `lookahead` and should be always started at zero.
>> 
>> But currently, the method `JavacParser#peekToken(int lookahead, Predicate<TokenKind>... kinds)` has not been used so that this issue doesn't effect the parser.
>> 
>> An alternative way:  remove the following methods, because they are not used.
>> 
>>     protected boolean peekToken(Predicate<TokenKind>... kinds)
>>     protected boolean peekToken(int lookahead, Predicate<TokenKind>... kinds) 
>> 
>> 
>> To test this patch, we can comment out the following methods  and  run the tests locally.
>> Note: some methods may need to add `@SuppressWarnings("unchecked")`.
>> 
>>     protected boolean peekToken(Predicate<TokenKind> tk1, Predicate<TokenKind> tk2, Predicate<TokenKind> tk3) {
>>         return peekToken(0, tk1, tk2, tk3);
>>     }
>> 
>>     protected boolean peekToken(int lookahead, Predicate<TokenKind> tk1, Predicate<TokenKind> tk2, Predicate<TokenKind> tk3) {
>>         return tk1.test(S.token(lookahead + 1).kind) &&
>>                 tk2.test(S.token(lookahead + 2).kind) &&
>>                 tk3.test(S.token(lookahead + 3).kind);
>>     }
>> 
>> 
>> Thanks for your review.
>> 
>> Best Regards,
>> -- Guoxiong
>
> Looks good - I wonder if we could rewrite all the other routines in terms of this one, rather than having a bunch of different related methods bottoming out in the same code?

@mcimadamore Thank you for your review.

These similar overloaded methods seem like a common way at module `jdk.compiler` and even the whole JDK. Is it related to the performance or other aspects?

I don't think it is good to remove them in this patch. Maybe we should continue to discuss this issue at another thread which is like `Using varargs methods to replace existing similar overloaded methods`.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4158