RFR: JDK-8223305: Compiler support for Switch Expressions
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Thu May 30 14:15:05 UTC 2019
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20190530/f1493b47/attachment-0001.html>
More information about the compiler-dev
mailing list