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

Wed Aug 1 14:04:16 UTC 2018

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"

- "break is missing a value to return from switch expression"

- 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?

- "incompatible types: bad type in switch expression"
...
(A cannot be converted to B)

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)

- "break is jumping outside of the enclosing switch expression" -> 
"break outside of enclosing switch expression"

- "continue is jumping outside of the enclosing switch expression" -> 
"continue outside of enclosing switch expression"

* 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)

* 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
>>