RFR: JDK-8192963/JDK-8206986 JEP 325: Switch Expressions (Preview)
Jan Lahoda
jan.lahoda at oracle.com
Thu Aug 9 11:59:17 UTC 2018
One more tiny change to tests to fix a failure on Windows (I apologize
for not folding that into the previous update):
webrev:
http://cr.openjdk.java.net/~jlahoda/8192963/webrev.04/
delta from previous (webrev.03):
http://cr.openjdk.java.net/~jlahoda/8192963/webrev.delta.03.04/
Thanks,
Jan
On 9.8.2018 13:39, Jan Lahoda wrote:
> 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