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

[lgxbslgx]

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

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8267580: The method JavacParser#peekToken is wrong when the first parameter is not zero

Reviewers

• Maurizio Cimadamore (@mcimadamore - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4158/head:pull/4158
$ git checkout pull/4158

Update a local copy of the PR:
$ git checkout pull/4158
$ git pull https://git.openjdk.java.net/jdk pull/4158/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4158

View PR using the GUI difftool:
$ git pr show -t 4158

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4158.diff

[bridgekeeper]

👋 Welcome back gli! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@lgxbslgx The following label will be automatically applied to this pull request:

• compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (56dc4364)

[mcimadamore]

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?

[openjdk]

@lgxbslgx This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: mcimadamore

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

• 3113910: 8267553: Extra JavaThread assignment in ClassLoader::create_class_path_entry()

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mcimadamore) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[lgxbslgx]

@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.

/integrate

[openjdk]

@lgxbslgx
Your change (at version 56dc436) is now ready to be sponsored by a Committer.

[mcimadamore]

I misread the patch. One method - peekToken(Predicate<TokenKind>... kinds) is already delegating to the one you fixed. And for the 1, 2, 3 token kind versions, one might argue that, by doing so we avoid array creation.

[mcimadamore]

/sponsor

[openjdk]

@mcimadamore @lgxbslgx Since your change was applied there has been 1 commit pushed to the master branch:

• 3113910: 8267553: Extra JavaThread assignment in ClassLoader::create_class_path_entry()

Your commit was automatically rebased without conflicts.

Pushed as commit 54520fb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.