RFR: JDK-8223305: Compiler support for Switch Expressions
Jan Lahoda
jan.lahoda at oracle.com
Thu May 30 13:58:06 UTC 2019
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