RFR: JDK-8192963/JDK-8206986 JEP 325: Switch Expressions (Preview)
Jan Lahoda
jan.lahoda at oracle.com
Thu Aug 9 11:39:21 UTC 2018
On 3.8.2018 22:39, Maurizio Cimadamore wrote:
> 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.
Sounds reasonable, I've updated the compiler.properties, and added one
more test for SimpleTreeVisitor.
An updated patch is here:
http://cr.openjdk.java.net/~jlahoda/8192963/webrev.03/
delta from the previous revision:
http://cr.openjdk.java.net/~jlahoda/8192963/webrev.delta.02.03/
Diagnostics:
http://cr.openjdk.java.net/~jlahoda/8192963/webrev.03/diags.html
Thanks,
Jan
>
> 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