RFR: JDK-8223305: Compiler support for Switch Expressions
Jan Lahoda
jan.lahoda at oracle.com
Thu Jun 6 14:11:39 UTC 2019
Hi,
Based on some more feedback, I've updated the patch with:
-a warning about the use of yield in cases where an error would be
reported if preview was enabled
-improvements to errors reported
-renames of a couple of methods, to better reflect what they do
Full patch:
http://cr.openjdk.java.net/~jlahoda/8223303/webrev.02/
Delta from previous iteration:
http://cr.openjdk.java.net/~jlahoda/8223303/webrev.delta.01.02/
How does this look?
Thanks,
Jan
On 30. 05. 19 16:19, Maurizio Cimadamore wrote:
> As for the unification, let's discuss that separately and let's focus on
> the first webrev for now. I think what I'd like to see there (if
> anything) is also an attempt to unify "findJumpTargetNoError" for both
> cases, so that we can use same logic in both visitYield and visitReturn.
>
> Maurizio
>
> On 30/05/2019 15:15, Maurizio Cimadamore wrote:
>>
>> Looks great! You only missed one rename in Resolve:
>>
>> private Symbol checkVarType(Symbol bestSoFar, Name name) {
>>
>> I guess that should be checkRestrictedType, or something like it?
>>
>> Maurizio
>>
>> On 30/05/2019 14:58, Jan Lahoda wrote:
>>> Hi,
>>>
>>> Thanks for all the comments so far. I've uploaded a new version of
>>> the patch here:
>>> http://cr.openjdk.java.net/~jlahoda/8223303/webrev.01/
>>> delta webrev from the previous state:
>>> http://cr.openjdk.java.net/~jlahoda/8223303/webrev.delta.00.01/
>>>
>>> The changes are:
>>> -adjustment to the updated spec, where yield is a restricted
>>> identifier (this required generalization of current error messages
>>> for var)
>>> -placing yield-related elements at the end for changes in
>>> com.sun.source, to better reflect alphabet order
>>> -simplification of error code from
>>> compiler.err.break.complex.value.no.switch.expression to
>>> compiler.err.no.switch.expression.
>>>
>>> This patch does not include unification of the
>>> AttrContext.returnResult/yieldResult, but I've done that here:
>>> http://cr.openjdk.java.net/~jlahoda/8223303/webrev.unified.attrcontext.result/
>>>
>>>
>>> There is a small issue in return handling, as return needs to look
>>> and the enclosing Envs to see if there is a switch expression or not.
>>> But if this looks OK, I can fold it into the main patch.
>>>
>>> Further feedback is welcome!
>>>
>>> Thanks,
>>> Jan
>>>
>>> On 28. 05. 19 12:09, Maurizio Cimadamore wrote:
>>>> Hi
>>>> I thought a bit more about the code (the Attr part) and I have few
>>>> more comments:
>>>>
>>>> * I don't immediately see the need for having another field in
>>>> AttrContext. After all we could rename/repurpose the existing
>>>> resultInfo.
>>>>
>>>> * of course, to do that, we need a way to detect whether we're
>>>> inside a switch expression, but that should be possible by looking
>>>> at the env? Or you could even exploit the check context, after all,
>>>> the check context for a case arm is always created in the
>>>> switchExpressionContext method.
>>>>
>>>> * it seems like when you are in visitYield, you should first check
>>>> if there's a target for the yield - and if there's not you log an
>>>> error. That will also remove some dependencies from yieldResult.
>>>>
>>>> Of course you can also leave the code as is. It just occurred that
>>>> having a separate ResultInfo specifically for yield (or break with
>>>> value, as the code was also there before) seemed a bit redundant.
>>>> But I can also believe that the current approach leads to more
>>>> sensible code.
>>>>
>>>>
>>>> Also, one small comment inline below.
>>>>
>>>>
>>>> On 28/05/2019 10:37, Jan Lahoda wrote:
>>>>> On 28. 05. 19 11:11, Maurizio Cimadamore wrote:
>>>>>> Looks good. Just few comments/questions:
>>>>>
>>>>> Thanks!
>>>>>
>>>>>>
>>>>>> * I think the error keys in compiler.properties could use some
>>>>>> renaming. e.g.
>>>>>>
>>>>>> compiler.err.break.complex.value.no.switch.expression ->
>>>>>> compiler.err.no.switch.expression
>>>>>> compiler.err.break.complex.value.no.switch.expression.qualify ->
>>>>>> compiler.err.no.switch.expression.qualify
>>>>>
>>>>> Sure, will do.
>>>>>
>>>>>>
>>>>>> * what is the new Log.hasErrorOn - and why has Flow been changed
>>>>>> to use it?
>>>>>
>>>>> Consider code like this:
>>>>> ---
>>>>> public class Y2 {
>>>>> private void t() {
>>>>> break;
>>>>> }
>>>>> }
>>>>> ---
>>>>>
>>>>> When compiled like this:
>>>>> javac -XDdev -XDshould-stop.at=FLOW Y2.java
>>>>
>>>> ah ok - it's the failover logic. I was trying to think of an example
>>>> w/o shouldStopAt and could not think of much.
>>>>
>>>> Thanks
>>>> Maurizio
>>>>
>>>>> It will crash:
>>>>> ---
>>>>> Y2.java:4: error: break outside switch or loop
>>>>> break;
>>>>> ^
>>>>> 1 error
>>>>> An exception has occurred in the compiler (11.0.3). Please file a
>>>>> bug against the Java compiler via the Java bug reporting page
>>>>> (http://bugreport.java.com) after checking the Bug Database
>>>>> (http://bugs.java.com) for duplicates. Include your program and the
>>>>> following diagnostic in your report. Thank you.
>>>>> java.lang.AssertionError
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.util.Assert.error(Assert.java:155)
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.util.Assert.check(Assert.java:46)
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.comp.Flow$AliveAnalyzer.visitMethodDef(Flow.java:518)
>>>>>
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:866)
>>>>>
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.tree.TreeScanner.scan(TreeScanner.java:49)
>>>>>
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.comp.Flow$BaseAnalyzer.scan(Flow.java:398)
>>>>>
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.comp.Flow$AliveAnalyzer.visitClassDef(Flow.java:488)
>>>>>
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.tree.JCTree$JCClassDecl.accept(JCTree.java:774)
>>>>>
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.tree.TreeScanner.scan(TreeScanner.java:49)
>>>>>
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.comp.Flow$BaseAnalyzer.scan(Flow.java:398)
>>>>>
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.comp.Flow$AliveAnalyzer.analyzeTree(Flow.java:759)
>>>>>
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.comp.Flow$AliveAnalyzer.analyzeTree(Flow.java:751)
>>>>>
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.comp.Flow.analyzeTree(Flow.java:216)
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1401)
>>>>>
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1375)
>>>>>
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:973)
>>>>>
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:311)
>>>>> at
>>>>> jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:170)
>>>>> at jdk.compiler/com.sun.tools.javac.Main.compile(Main.java:57)
>>>>> at jdk.compiler/com.sun.tools.javac.Main.main(Main.java:43)
>>>>> ---
>>>>>
>>>>> The reason is that javac asserts that it has properly processed the
>>>>> exits - but here the original code is broken, and an error has
>>>>> already been reported and this given spot, so it seems safe to not
>>>>> crash javac here.
>>>>>
>>>>> Thanks,
>>>>> Jan
>>>>>
>>>>>>
>>>>>> Thanks
>>>>>> Maurizio
>>>>>>
>>>>>>
>>>>>> It seems like a whitespace got remove here?
>>>>>>
>>>>>> On 24/05/2019 15:48, Jan Lahoda wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> I'd like to ask for a review of changes to update javac to follow
>>>>>>> the current spec for switch expressions, in particular the break
>>>>>>> -> yield change:
>>>>>>> http://cr.openjdk.java.net/~gbierman/jep354-jls-20190524.html
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~jlahoda/8223303/webrev.00/
>>>>>>>
>>>>>>> JBS:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8223305
>>>>>>>
>>>>>>> Feedback is welcome!
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jan
More information about the compiler-dev
mailing list