RFR: JDK-8192963/JDK-8206986 JEP 325: Switch Expressions (Preview)

[Jonathan Gibbons]

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