On Tue, 3 Oct 2023 09:58:11 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org>
wrote:
>> Aggelos Biboudis has updated the pull request with a new target base due to
>> a merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into primitive-patterns
>> - Implement type pairs to exactnessMethod name
>> - Apply suggestions from code review
>>
>> Co-authored-by: Raffaello Giulietti <raffaello.giulie...@oracle.com>
>> - 8303374: Compiler Implementation for Primitive types in patterns,
>> instanceof, and switch (Preview)
>
> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 228:
>
>> 226: currentTest = trueDef;
>> 227: } else if (currentLabelClass.isPrimitive()) {
>> 228: if (selectorType.isInstance(Object.class)) {
>
> As discussed offline, I believe this is wrong, and it should be replaced by
> `Class::equals`. That said, I think that "accidentally" you get the desired
> behavior here, as demonstrated by jshell:
>
>
> jshell> Byte.class.isInstance(Object.class)
> $6 ==> false
>
>
> jshell> Object.class.isInstance(Object.class)
> $8 ==> true
>
>
> So, this effectively acts as a test to check if the selector type is
> `Object`. Of course, since `isInstance` is used, spurious stuff is picked up
> as well:
>
>
> jshell> Class.class.isInstance(Object.class)
> $7 ==> true
>
> But this situation turns out to be non problematic, given that a primitive
> pattern is not applicable to a selector type Class.
Fixed as well.
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 5052:
>
>> 5050:
>> 5051: return (source.isPrimitive() && target.isPrimitive()) &&
>> 5052: ((source.hasTag(BYTE) && !target.hasTag(CHAR) ||
>
> Does this mean that `byte` -> `char` is not exact? Aren't both integral
> types, thus invalidating `widening from one integral type to another` ?
Indeed it is not unconditionally exact. byte to char combines both widening and
narrowing primitive conversions
https://docs.oracle.com/javase/specs/jls/se8/html/jls-5.html#jls-5.1.4
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4154:
>
>> 4152: public void visitTypeTest(JCInstanceOf tree) {
>> 4153: Type exprtype = attribExpr(tree.expr, env);
>> 4154: if(!allowPrimitivePatterns) {
>
> nit: space missing
Fixed!
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 735:
>
>> 733: }
>> 734: }
>> 735: if (tree.selector.type.hasTag(TypeTag.BOOLEAN)) {
>
> I would have expected true/false to be treated more or less like enum
> constants when it came to exhaustiveness? E.g. boolean is similar to an enum
> type that only has two options (or, if you will, a sealed type with two
> permitted subtypes). So, IMHO the treatment for booleans should happen in the
> `exhausts` method - and this method should be left very simple (possibly
> unchanged?)
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1344099605
PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1344095080
PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1344095209
PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1344095335
