On Thu, 31 Jul 2025 22:31:55 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> This issue is still unresolved: `CoreSymbols.getFunctions()` is a collection
>> of `Function`, but it's searched for a `String` (.getName()).
>>
>> The code in lines 216 and 238 needs to be fixed.
>>
>> I suspect we are missing a unit test that exercises this functionality,
>> because the code should never work. How did it work?
>
> Andy and Nir are right. I don't see how the following can do what you intend:
>
>
> if
> (!CoreSymbols.getFunctions().contains(getFuncName(e.getFunction().getName()))
> &&
>
>
> Since `CoreSymbols.getFunctions()` is a `Set<Function>` and
> `e.getFunction().getName()` is a `String`, the `contains` will always return
> false (meaning this term in the `if` expression is always true).
>
> Can you check this?
The check was incorrect and the `contains()` call always returned `false`.
Due to this incorrect `contains()` call, I would have created the
`libraryFunctionsUsedInShader` Set. `libraryFunctionsUsedInShader` is a subset
of `CoreSymbols.getFunctions()`. So, after correcting the above `contains`
call, the `libraryFunctionsUsedInShader` set is not required.
The previous if condition
if
(!CoreSymbols.getFunctions().contains(getFuncName(d.getFunction().getName())) &&
!libraryFunctionsUsedInShader.contains(getFuncName(d.getFunction().getName())))
is now changed to:
if (!CoreSymbols.getFunctions().contains(d.getFunction()))
The wrong check did not cause any issue, as the check
`!CoreSymbols.getFunctions().contains(getFuncName(d.getFunction().getName()))`
was wrong but always `true` and
`!libraryFunctionsUsedInShader.contains(getFuncName(d.getFunction().getName()))'
check was sufficient enough for achieving required result.
The metal shaders generated before and after this change are exactly same.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1824#discussion_r2246784569
