On Wed, 29 Oct 2025 13:48:04 GMT, Jan Lahoda <[email protected]> wrote:
>> Aggelos Biboudis has refreshed the contents of this pull request, and
>> previous commits have been removed. The incremental views will show
>> differences compared to the previous content of the PR. The pull request
>> contains one new commit since the last revision:
>>
>> 8359145: JEP 530: Implement Primitive Types in Patterns, instanceof, and
>> switch (Fourth Preview)
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 4743:
>
>> 4741: boolean dominated = false;
>> 4742:
>> 4743: if (unconditionalCaseLabel == testCaseLabel)
>> unconditionalFound = true;
>
> I think the code here is correct, but it is very hard to read (to me, at
> least). I wonder if there's a chance to try to tweak the code so that it is
> more readable.
>
> At least: do `{}` around `unconditionalFound`, and move the
> `dominatedCandidate` into the `if (!currentType.hasTag(ERROR) &&
> !testType.hasTag(ERROR)) {`. I suspect it might be possible to replace
> `dominatedCandidate` with `continue` in `if (currentType.constValue()
> instanceof Number) {` (would need to `if (allowPrimitivePatterns &&
> unconditionalFound && unconditionalCaseLabel != label) {` check upfront, so
> not sure if that would improve things).
I simplified the code (abstained from using `continue`). Do you still think it
is necessary?
> test/langtools/tools/javac/patterns/PrimitiveUnconditionallyExactInAssignability.java
> line 93:
>
>> 91: }
>> 92: void assertOriginalAssignmentNarrowingAndUnconditionality() {
>> 93: // byte b = <constant short> vs
>> ExactConversionsSupport::isIntToByteExact
>
> If I understand it correctly, the updated and original isAssignable should
> give the same results, right? I think it would be better to have one
> invocation, like `assertOriginaAndUpdatedAssignable`, that would check that
> for the given input, both methods return the same value, and the expected
> value. Having to lists of cases is likely to lead to omissions/mistakes, I
> suspect.
Addressed!
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27637#discussion_r2478811104
PR Review Comment: https://git.openjdk.org/jdk/pull/27637#discussion_r2478812442
