RFR: JDK-8192963/JDK-8206986 JEP 325: Switch Expressions (Preview)
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Thu Aug 9 12:53:54 UTC 2018
Looks good.
Maurizio
On 09/08/18 12:59, Jan Lahoda wrote:
> 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