RFR: JDK-8192963/JDK-8206986 JEP 325: Switch Expressions (Preview)
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Fri Aug 3 20:39:48 UTC 2018
Looks good - when I eyeballed compiler.properties earlier I missed this:
"value break not supported in 'enhanced for' "
I understand why you get 'enhanced for', not sure that adds value w.r.t.
just 'for'. I don't have a strong opinion.
Maurizio
On 03/08/18 21:26, Jan Lahoda wrote:
> 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