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