RFR: JDK-8192963/JDK-8206986 JEP 325: Switch Expressions (Preview)
Jan Lahoda
jan.lahoda at oracle.com
Fri Aug 3 13:35:04 UTC 2018
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