RFR: JDK-8192963/JDK-8206986 JEP 325: Switch Expressions (Preview)
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Fri Aug 3 13:47:50 UTC 2018
I like the diagnostic changes, and the API change - I think it's more
consistent now - thanks for taking a second look.
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