[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-compiler-dev
Subject: Re: RFR: JDK-8223305: Compiler support for Switch Expressions
From: Maurizio Cimadamore <maurizio.cimadamore () oracle ! com>
Date: 2019-06-06 17:49:56
Message-ID: 9453bdf1-dc45-1b0d-5713-c9d688f43668 () oracle ! com
[Download RAW message or body]
Looks good!
Maurizio
On 06/06/2019 15:11, Jan Lahoda wrote:
> 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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic