On 08/21/2017 08:17 PM, Aleksey Shipilev wrote:
> On 08/21/2017 08:06 PM, Paul Sandoz wrote:
>>> On 21 Aug 2017, at 07:48, Claes Redestad <claes.redes...@oracle.com> wrote:
>>> a trivial test[1] invoking the StringConcatFactory.makeConcatWithConstants
>>> fails
>>> when providing an Integer as a constant, which appears to be due to failure
>>> to
>>> coerce boxed types to the corresponding primitive types when looking up
>>> various
>>> methods in StringConcatHelper:
>>>
>>> Webrev: http://cr.openjdk.java.net/~redestad/8186500/jdk.00/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8186500
>>>
>>> Simply using Wrapper.asPrimitiveType coerces boxed types to their primitive
>>> counterpart, and is a (semantical) no-op for other types, e.g., String.
>>
>> Looks good. Perhaps a token test would be useful (maybe hard to test all
>> code paths here without some combinator test).
>
> Please add tests. We have to at least assert that passing "null" works fine
> there -- I am not sure
> it does right now. Since we are talking non-String constants, passing
> java/lang/Object is a good
> test too.
I remembered why we haven't covered this before:
* <li><em>\2 (Unicode point 0002):</em> a constant. This input passed
* through static bootstrap argument. This constant can be any value
* representable in constant pool. If necessary, the factory would call
* {@code toString} to perform a one-time String conversion.</li>
Neither Integer, nor Object are representable in constant pool, and so this bug
cannot surface via
JDK 9 bytecode, but only via direct SCF call. So, from some point of view, SCF
may exhibit undefined
behavior, including throwing up on non-CP-representable value.
Using "should be the value representable in constant pool" instead of "can..."
would make it
stronger, but alas. Once other constant flavors are added (/me looks at Paul,
Brian and John), it
would become a possibility. This is why I think we need to add Integer and
Object constant tests for
future proofing SCF, while this is still a gray area.
...and maybe add {Class, MethodHandle, MethodType} as the tested arguments too,
as per:
https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.4
Thanks for taking care of this!
-Aleksey
