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