RFR: JDK-8192963/JDK-8206986 JEP 325: Switch Expressions (Preview)

Jan Lahoda jan.lahoda at oracle.com
Fri Aug 3 20:26:34 UTC 2018 

On 3.8.2018 16:22, Jan Lahoda wrote:
> On 3.8.2018 15:47, Maurizio Cimadamore wrote:
>> I like the diagnostic changes, and the API change - I think it's more
>> consistent now - thanks for taking a second look.
>
> Thanks!
>
> I've forgot to include the generated examples for this update, so I've
> placed it here:
> file:///usr/local/home/lahvac/src/jdk/cr.openjdk.java.net/8192963/webrev.02/diags.html

I apologize for a wrong link, the correct one is:
http://cr.openjdk.java.net/~jlahoda/8192963/webrev.02/diags.html

Jan

>
>
> Jan
>
>>
>> Thumbs up.
>>
>> Maurizio
>>
>>
>> On 03/08/18 14:35, Jan Lahoda wrote:
>>> Hi,
>>>
>>> Thanks for all the comments. An updated patch is here:
>>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.02/
>>> Changes against the previous revision:
>>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.delta.01.02/
>>>
>>> Updated specdiff:
>>> http://cr.openjdk.java.net/~jlahoda/8192963/api.diff.02/overview-summary.html
>>>
>>>
>>>
>>> Any feedback is welcome.
>>>
>>> Thanks,
>>>     Jan
>>>
>>> On 1.8.2018 18:39, Maurizio Cimadamore wrote:
>>>>
>>>>
>>>> On 01/08/18 16:43, Jan Lahoda wrote:
>>>>> Hi Maurizio,
>>>>>
>>>>> Thanks for the comments - responses are inlined.
>>>>>
>>>>> On 1.8.2018 16:04, Maurizio Cimadamore wrote:
>>>>>> Hi Jan,
>>>>>> great work, some comments below.
>>>>>>
>>>>>> * Diagnostics. I see that some diagnostics seem to be inconsistent
>>>>>> with
>>>>>> what we do for 'return' - e.g. javac diagnostics for 'return' are
>>>>>> quite
>>>>>> succint:
>>>>>>
>>>>>> - "return outside method"
>>>>>>
>>>>>> - "missing return statement"
>>>>>>
>>>>>> - "unexpected return value"
>>>>>>
>>>>>> - "error: incompatible types: String cannot be converted to int"
>>>>>>
>>>>>> The analogous conditions for break in switch expression seem to be
>>>>>> more
>>>>>> verbose:
>>>>>>
>>>>>> - "expression break not immediately enclosed by a switch expression"
>>>>>
>>>>> I think the closest error to "return outside method" is "value break
>>>>> outside of switch expression". The "expression break not immediately
>>>>> enclosed by a switch expression" is used when the break is inside
>>>>> another breakable statement inside the switch expression. I guess this
>>>>> should be rephrased at least to "value break not immediately enclosed
>>>>> by switch expression".
>>>>>
>>>> Fine, but do we need two distinct errors? Or do we need maybe the same
>>>> error and an extra detail string at the bottom for the 'you are
>>>> inside a
>>>> switch expr, but there's some other breaky context in between'
>>>> condition?
>>>>>>
>>>>>> - "break is missing a value to return from switch expression"
>>>>>
>>>>> How about
>>>>> "missing break value"?
>>>>>
>>>>> (the break is present, but the value is missing.)
>>>> I'm fine with that!
>>>>>
>>>>>>
>>>>>> - There's no equivalent for the third above - probably the closest
>>>>>> condition would be to 'break expr' from a switch statement, in which
>>>>>> case I bet you get a 'missing label' error - should we rectify that
>>>>>> too?
>>>>>
>>>>> There are several conditions, and the error differ, consider:
>>>>> ---
>>>>> public class SwitchExpr {
>>>>>       private void breakTest(int e) {
>>>>>           break ;
>>>>>           break x;
>>>>>           break (e);
>>>>>           while (true) {
>>>>>                break x;
>>>>>                break (e);
>>>>>           }
>>>>>       }
>>>>> }
>>>>> ---
>>>>>
>>>>> This gives:
>>>>> ---
>>>>> SwitchExpr.java:3: error: break outside switch or loop
>>>>>           break ;
>>>>>           ^
>>>>> SwitchExpr.java:4: error: undefined label: x
>>>>>           break x;
>>>>>           ^
>>>>> SwitchExpr.java:5: error: value break outside of switch expression
>>>>>           break (e);
>>>>>           ^
>>>>> SwitchExpr.java:7: error: undefined label: x
>>>>>                break x;
>>>>>                ^
>>>>> SwitchExpr.java:8: error: value break outside of switch expression
>>>>>                break (e);
>>>>>                ^
>>>>> 5 errors
>>>>> ---
>>>>>
>>>>> So when it is clear the break is a value break, a value break related
>>>>> diagnostic is reported, otherwise, the errors fall back to the
>>>>> existing diagnostics of cannot find labels.
>>>>>
>>>>>>
>>>>>> - "incompatible types: bad type in switch expression"
>>>>>> ...
>>>>>> (A cannot be converted to B)
>>>>>
>>>>> I think this was modelled based on conditional expressions:
>>>>> SwitchExpr.java:10: error: incompatible types: bad type in conditional
>>>>> expression
>>>>>           B b = e == 0 ? (A) null : (B) null;
>>>>>                          ^
>>>>>     A cannot be converted to B
>>>>>
>>>>> I'll look if this can be changed.
>>>> I see, I'm not a big fan of the conditional expression nesting too - I
>>>> think the important part is 'A != B' and it should be put front and
>>>> center. That said, this is not a blocking issue, and I'm ok with a
>>>> followup.
>>>>>
>>>>>>
>>>>>> Not sure whether we should try to maintain the same style across
>>>>>> break/return, but it seemed worth at least pointing out.
>>>>>>
>>>>>> Other diagnostic comments:
>>>>>>
>>>>>> - "error: j is ambiguously both a valid expression and a valid
>>>>>> label" -
>>>>>> this seems convoluted, I would personally rephrase as something like:
>>>>>>
>>>>>> ambiguous reference to 'j'
>>>>>> ...
>>>>>> ('j' is both a label and an expression)
>>>>>
>>>>> Will done.
>>>>>
>>>>>>
>>>>>> - "break is jumping outside of the enclosing switch expression" ->
>>>>>> "break outside of enclosing switch expression"
>>>>>
>>>>> Will do.
>>>>>
>>>>>>
>>>>>> - "continue is jumping outside of the enclosing switch expression" ->
>>>>>> "continue outside of enclosing switch expression"
>>>>>
>>>>> Will do.
>>>>>
>>>>>>
>>>>>> * Attr - The refactoring of switchExpr into switch + handleSwitch is
>>>>>> very sweet - I like it!
>>>>>>
>>>>>> * Flow - a bit odd that we have to make up a new tree to call
>>>>>> recordExit
>>>>>> - but I see why you're doing it :-)
>>>>>>
>>>>>> * Lower - it would be nice to have a comment on SwitchExpression that
>>>>>> shows what the generated code shape looks like; I think we do that
>>>>>> for
>>>>>> other non-trivial translations (e.g. for each)
>>>>>
>>>>> Will do.
>>>> Thanks
>>>> Maurizio
>>>>>
>>>>> Thanks,
>>>>>     Jan
>>>>>
>>>>>>
>>>>>> * ExpSwitchNestingTest - I see that this test is not using the newer
>>>>>> combo framework. Actually, I'm not even sure this one needs any
>>>>>> templating logic at all, as it seems the test is doing its own
>>>>>> replacements. But I was going to note that this test is not using the
>>>>>> shared context enhancements, which I guess it's still ok given the
>>>>>> number of combinations checked is low. Maybe worth keeping an eye on
>>>>>> this in the future, should we keep adding new cases to it.
>>>>>>
>>>>>> Maurizio
>>>>>>
>>>>>> On 01/08/18 07:28, Jan Lahoda wrote:
>>>>>>> Hi Jon,
>>>>>>>
>>>>>>> Thanks for the comments!
>>>>>>>
>>>>>>> An updated webrev that reflects the comments is here:
>>>>>>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.01/
>>>>>>>
>>>>>>> A delta webrev between this webrev and the webrev.00 is here:
>>>>>>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.delta.00.01/
>>>>>>>
>>>>>>> Does this look better?
>>>>>>>
>>>>>>> Thanks!
>>>>>>>
>>>>>>> Jan
>>>>>>>
>>>>>>> On 28.7.2018 01:12, Jonathan Gibbons wrote:
>>>>>>>> CaseTree.java:52: spelling, "lables"
>>>>>>>>
>>>>>>>> CaseTree.java:84:  "return" should be "returns".
>>>>>>>>
>>>>>>>> Lots of @Deprecated annotations without corresponding @deprecated
>>>>>>>> javadoc tag.
>>>>>>>> Should the @apiNote really be @deprecated.  I also think that using
>>>>>>>> @Deprecated
>>>>>>>> like this is "a bit smelly", and had a discussion with Stuart "Dr
>>>>>>>> Deprecator" regarding
>>>>>>>> my concerns. If nothing else, assuming the feature is a success, we
>>>>>>>> are
>>>>>>>> setting up
>>>>>>>> a precedent of marking an API as deprecated-for-removal, and then
>>>>>>>> in the
>>>>>>>> next
>>>>>>>> release, simply removing the annotation.
>>>>>>>>
>>>>>>>> Attr.java:1482: ugly use of @SuppressWarnings. I understand why
>>>>>>>> it is
>>>>>>>> there,
>>>>>>>> but it exemplifies why @Deprecated on the declaration is a bad
>>>>>>>> idea.
>>>>>>>> It'll be too easy to not remove these annotations in JDK 13.
>>>>>>>>
>>>>>>>> Attr:1825: why XXX
>>>>>>>>
>>>>>>>> LambdaToMethod: unnecessary new import, CaseKind
>>>>>>>> Flow: unnecessary new import, CaseKind
>>>>>>>>
>>>>>>>> JavacParser: order of imports
>>>>>>>>
>>>>>>>> messages: generally, I think the text of the messages could be
>>>>>>>> improved
>>>>>>>> (related but not new) we should somehow identify language keywords
>>>>>>>> from
>>>>>>>> English text (i.e. words like `break, `case`, `continue`, `return`)
>>>>>>>> 204: saying the name is both a valid label and a valid expression
>>>>>>>> hints
>>>>>>>> at the cause but not the error (i.e. the name is ambiguous)
>>>>>>>>
>>>>>>>> JCTree: inconsistent layout of annotations; inconsistent use of
>>>>>>>> @Deprecated and @Deprecated(forRemoval=true)
>>>>>>>>
>>>>>>>> (tests)
>>>>>>>> Not reviewed in detail yet.
>>>>>>>> New test descriptions should use "/*" for jtreg comment block; not
>>>>>>>> "/**". These are not javadoc documentation comments.
>>>>>>>> New test descriptions should have @summary
>>>>>>>> Presumably, we will have to change the -old.out files if the
>>>>>>>> preview
>>>>>>>> feature becomes standard in JDK 13.
>>>>>>>>
>>>>>>>>
>>>>>>>> Overall,
>>>>>>>>
>>>>>>>> The logic is nice, but the use of @Deprecated(forRemoval=true) and
>>>>>>>> resulting @SuppressWarnings really suck.
>>>>>>>> I don't think the current practice is serving us well.
>>>>>>>>
>>>>>>>> -- Jon
>>>>>>>>
>>>>>>>>
>>>>>>>> On 07/17/2018 11:00 AM, Jan Lahoda wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> As JEP 325 is in the proposed to target state, I thought it might
>>>>>>>>> be a
>>>>>>>>> good idea to start a review process for the code.
>>>>>>>>>
>>>>>>>>> The code here strives to implement the current specification for
>>>>>>>>> the
>>>>>>>>> Switch Expressions:
>>>>>>>>> http://cr.openjdk.java.net/~gbierman/switch-expressions.html
>>>>>>>>>
>>>>>>>>> The current webrev is here:
>>>>>>>>> http://cr.openjdk.java.net/~jlahoda/8192963/webrev.00/
>>>>>>>>>
>>>>>>>>> (includes a list of new errors.)
>>>>>>>>>
>>>>>>>>> JBS:
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8192963
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8206986
>>>>>>>>>
>>>>>>>>> CSRs:
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8207241
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8207406
>>>>>>>>>
>>>>>>>>> Any feedback is welcome!
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>     Jan
>>>>>>>>
>>>>>>
>>>>
>>

More information about the compiler-dev mailing list