New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8057543: Replace javac's Filter with Predicate (and lambdas) #1898
Conversation
|
/label remove kulla |
@lgxbslgx |
More information and some similar issues: These four issues were fixed in this patch This issue was fixed in this patch But their status are still |
@lgxbslgx This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
@lgxbslgx This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good - sorry I missed it. If you reopen I will approve!
@mcimadamore Thank you for your help. I will reopen it. |
I can't find the |
Mailing list message from Maurizio Cimadamore on compiler-dev: Not sure - I don't see it either - CC'ing Erik Maurizio On 06/04/2021 06:13, Guoxiong Li wrote: |
@mcimadamore Who should we ping to solve the |
@edvbld Can you please help? |
/open |
@lgxbslgx @HostUserDetails{id=13688759, username='lgxbslgx', fullName='null'} this pull request is now open |
@mcimadamore The PR could be reopen by typing |
# Conflicts: # src/jdk.compiler/share/classes/com/sun/tools/javac/parser/Tokens.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - left some comments
@@ -1,5 +1,5 @@ | |||
/* | |||
* Copyright (c) 2006, 2014, Oracle and/or its affiliates. All rights reserved. | |||
* Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copyright in this (and other files need to be updated)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -90,7 +91,7 @@ | |||
|
|||
final Symtab syms; | |||
final JavacMessages messages; | |||
final Names names; | |||
private Names names; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this change coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When building the JDK, I got the following error:
jdk/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java:950: error: variable names might not have been initialized
t.name != names.init &&
^
1 error
Therefore I revised the modifier of the Names names
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - honestly this seems like a bug in definite assignment? E.g. surely the instance field initializer for bridgeFilter
must run after the final
field names
has been initialized? For now you might try using an anonymous inner class instead of a lambda instead, pretty sure that will take care of the issue. But this looks like a compiler bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually - doing more tests, I think javac is right. Fields with explicit initializers are initialized BEFORE other constructor statements. Which means that the error message is legitimate (e.g. the lambda is capturing a final value that is not initialized). I think reverting back to anon class would be better here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a bug, it's a dubious feature, the lambda EG discussed that at length, you can ask Brian about it.
Usually replacing
t.name != names.init
by
t.name != this.names.init
is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@forax Thank you for your reply. I tested t.name != this.names.init
with final Names names;
just now. The error also occurred. Same as t.name != names.init
.
And I think that t.name != names.init
and t.name != this.names.init
are equal at this situation.
*/ | ||
@Deprecated(forRemoval = true, since = "17") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a public API - we're in a JDK internal class - if this class is no longer needed (as is the goal of this change) simply remove it :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@mcimadamore I want to set these issues to |
Mailing list message from Maurizio Cimadamore on compiler-dev: Not sure - it doesn't seem to me that the patch for JDK-8057547 converts Maurizio On 21/04/2021 16:36, Guoxiong Li wrote: |
1 similar comment
Mailing list message from Maurizio Cimadamore on compiler-dev: Not sure - it doesn't seem to me that the patch for JDK-8057547 converts Maurizio On 21/04/2021 16:36, Guoxiong Li wrote: |
@mcimadamore Thank you for your reply about these issues. For JDK-8057542
For JDK-8057545
For JDK-8057546
For JDK-8057547
On the other hand, I search |
I agree that all these issues have been fixed - sorry - I thought these issues wanted to replace the various interfaces with interfaces from |
@mcimadamore OK, I closed them just now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@lgxbslgx This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
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 24 new commits pushed to the
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.
|
@mcimadamore Thank you for your review. Could I get your help to sponsor this patch? /integrate |
/sponsor |
@mcimadamore @lgxbslgx Since your change was applied there have been 25 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 657f103. |
Hi all,
This patch replaces javac's
Filter
withPredicate
and setsFilter
asDeprecated
.Two existing tests failed and I fixed it.
Currently, all the tests in
test/langtools
passed locally by using the following command.Thank you for taking the time to review.
Best Regards.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/1898/head:pull/1898
$ git checkout pull/1898
Update a local copy of the PR:
$ git checkout pull/1898
$ git pull https://git.openjdk.java.net/jdk pull/1898/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1898
View PR using the GUI difftool:
$ git pr show -t 1898
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/1898.diff