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

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Wed Aug 1 16:39:59 UTC 2018 


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