Parser changes for JEP 443 #1517
Conversation
|
@mpalat Hi Manoj: We were looking for feedback on this draft PR while we work to get all the tests green. Thanks in advance. |
|
waiting on #1523 to be merged for many of the tests to be fixed. |
datho7561
force-pushed
the
893-jep-443
branch
from
baa7215 to
b366cad
Compare
|
This change looks pretty good to me.
HTH |
datho7561
force-pushed
the
893-jep-443
branch
2 times, most recently
from
0a69259 to
267f76d
Compare
|
@mpalat Would you please review this one? |
sure.will take a look First of all, the grammar change - java.g. I think there are too many changes here to accommodate the underscore. An alternate approach to try out is to accept the underscore in the parser, and in some way use the mechanism similar to var type. Could you please try out this alternate? - like how @mickaelistria is doing in his patch - accept a larger set for unnammed and then flag errors at a later stage rather than having many changes in th java.g Also, I thinks its better to cut down into incremental patches similar to those in #1106 |
|
Hi Manoj. Thanks for the review. I think I found a way to reduce the number of changes necessary in |
@datho7561 - That should be fine - I was experimenting with something similar here as shown below: However, I would need to see those changes to comment further- please provide the changes for me to take a look |
datho7561
force-pushed
the
893-jep-443
branch
8 times, most recently
from
6c07fc4 to
5274544
Compare
|
@mpalat Alright, I've pushed the changes so that less work is done in |
Noting that I am away from work (or supposed to) for the last couple of days not being in the best of health, am just looking at this as health permits. Also my time is limited in the next few weeks - in short the reviews may not happen fast. That said, to have more number of (very good) eyes look at it and to continue the review, have discussed with @srikanth-sankaran and he has agreed to help out with the new patches once he is finished reviewing String templates this is being implemented by Jay with inputs from Srikanth. And ideally it would be good to take it one by one since both Unnammed Classes and this have java.g changes and that we need to really concentrate on one problem of grammar change at a time to evolve a minimal, maintainable change. I would suggest to wait for @mickaelistria unnamed class and then take this patch unnammed patterns . Both of us (@srikanth-sankaran and I ) felt that the grammar changes should be very minimal in the unnammed patterns - much less than the java.g changes listed in the patch, but we will have to scrutinize/try it out to be sure. |
|
@mpalat Hello Manoj, is it possible for you to review this PR sometime soon? Mickael and I are still working on getting the tests for the Unnamed Classes grammar change PR to pass, whereas on this PR they are passing. |
|
@mpalat Thanks for considering the unnamed class as higher priority, but at the moment, I think the patch is not yet ready for review (as long as tests are stuck or some are failing), so I would recommend for the best interest of the project that you remove the unnamed class patch for your current monitoring and instead consider the unnamed variable patch which is assumed complete already. |
|
@srikanth-sankaran when you have time, could you please review this PR? |
datho7561
force-pushed
the
893-jep-443
branch
from
5274544 to
bbaa65d
Compare
Yes, I will, thanks. |
Thanks @srikanth-sankaran. @datho7561 as informed earlier, am tied up next few days, however will work with Srikanth as time permits |
|
I am Okay with the changes in the specific set of files I looked at. I still see some requested from Srikanth pending. Once they are resolved, we are good to go. |
@datho7561, please go over all the past comments to make sure everything is addressed. Once you confirm, I'll make a sanity check and help close this |
datho7561
force-pushed
the
893-jep-443
branch
from
83d9d1d to
8af9b30
Compare
|
The main thing that isn't addressed is this bug in code generation that I created a separate PR for: #1816 Other than that it should be good. |
|
A heads up that fundamental restructuring of pattern matching support is happening via #1885 which should get merged in a day or two. I'll see what if any changes would be needed to harmonize this work with that PR and help align them. |
This is done and if you are update your changes, I can take it forward. Thanks |
datho7561
force-pushed
the
893-jep-443
branch
from
94b354d to
0e58669
Compare
|
OK, I am starting a quick pass over this and will try to dedicate the next 2-3 days to this and see if this can be merged. |
|
I am having a tough time rebasing to master - I know this has been a recurring exercise and I wanted to take it off of your hands. But having a tough time, hitting various parser generator bugs ... I will continue to look into this, in the meantime if you see this message and can spare the cycles to update against master and force-push, that would be super |
OK, I have figured out the parser generator breakage. But merge is still proving to be unintutive. I think I'll squash all commit into one and then rebase. This may result in a simpler workflow |
No, spoke too soon it seems. It was working for a while but now I get various errors while running the parser generator scripts: |
|
OK, I am giving up on the rebase/merge conflicts/ approach. I will review the PR as is. Before merging it needs to be brought to uptodate state anyway. This you can handle. |
Summary: I am not blocked. I am reviewing by comparing your branch HEAD against master as opposed to rebase and then compare with previous version. It should all wash out |
| public boolean isUnnamed(BlockScope scope) { | ||
| return ((this.name.length == 1 && this.name[0] == '_') || ("\\u005F".equals(this.name.toString()))) //$NON-NLS-1$ | ||
| && scope.compilerOptions().sourceLevel >= ClassFileConstants.JDK21 | ||
| && scope.compilerOptions().enablePreviewFeatures; |
There was a problem hiding this comment.
Curious - is the ("\\u005F".equals(this.name.toString()))) really required ? Won't the scanner internalize the unicode and return an '_' ??
| if ((length = this.withoutUnicodePtr) == 1) { | ||
| if (this.withoutUnicodeBuffer[0] == '_') { | ||
| return TokenNameUNDERSCORE; | ||
| } |
There was a problem hiding this comment.
This is what I meant. When an underscore occurs as unicode, the scanner converts that to '_' - No ??
There was a problem hiding this comment.
Ok, I have made a full pass over the changes and exhausted all questions and comments. Please proceed to
- Rebase against master - there are some more pattern related changes on master that need to be carefully picked up.
- I have provided a comment about unicode - it is more a nit and academic curiosity actually. Please take a look.
- Do you swear under penalty of perjury that you have tested every snippet in
org.eclipse.jdt.core.tests.compiler.regression.UnnamedPatternsAndVariablesTestand in general every modified test to compare behavior withJDK21?? :)
If all this is addressed, we are ready to merge this into master.
|
FYI, I have been drastically overhauling our patterns implementation to make it simpler to understand and maintain: See: #1884 - [Patterns] Alternate, simpler pattern binding management implementation There is plenty more particularly in the area of code generation that will change. An alert that your work will also come under this comprehensive overhaul. If you wish to be notified, I can CC you on the various tickets. @iloveeclipse - FYI |
|
This one is ready to merge - let us move forward please. I am aware that you have had to repeatedly rebase and resolve conflicts - what can look like a thanksless exercise sometimes. Really appreciate your patience @datho7561 ! |
|
So everybody is on same page: I am listing the next steps for the 4 PRs @datho7561 has been working on:
|
|
Please rebase and squash all intermediate commits to one before merging. |
Contains support for unnamed patterns and variables (JEP443), including parsing, analysis and code generation, but omits support for lists of patterns in case statements, which will be handled in a future PR. Signed-off-by: David Thompson <[email protected]>
datho7561
force-pushed
the
893-jep-443
branch
from
0e58669 to
22fcccc
Compare
|
Alright, I've rebased the branch, squashed the commits, addressed the comment about Unicode support (you're right, I didn't need that check), and double checked I have all the snippets from the JEP (excluding those dealing with lists of patterns). |
…e-jdt#1517) Contains support for unnamed patterns and variables (JEP443), including parsing, analysis and code generation, but omits support for lists of patterns in case statements, which will be handled in a future PR. Signed-off-by: David Thompson <[email protected]>
What it does
Support parsing unnamed variables and unnamed patterns.
How to test
Create a Java 21 project with preview features enabled. Ensure that the parser can parse unnamed variables and unnamed patterns as described in https://openjdk.org/jeps/443.
Author checklist