RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants

[Claes Redestad]

Right,

the Wrapper.* code appears to work fine, but makeConcatWithConstants has
pre-existing issues with non-primitive, non-String constants.

What I wasn't sure about is *when* the String.valueOf should happen, but as
makeConcatWithConstants specify "If necessary, the factory would call 
toString to
perform a one-time String conversion" then I think we could (should?) do 
this at
our earliest convenience, which might even be in the RecipeElement 
construction:

http://cr.openjdk.java.net/~redestad/8186500/jdk.02/

Reworked the test to be easier to add constant types. Null constants are 
specified
to throw NPE, so the test now checks that (this might be a needlessly 
strict thing
to specify, but changing the method specification is out of scope here, 
I think).

WDYT?

/Claes

On 08/22/2017 08:49 PM, Aleksey Shipilev wrote:
> On 08/22/2017 07:10 PM, Claes Redestad wrote:
>> On 2017-08-22 18:34, Aleksey Shipilev wrote:
>>>> http://cr.openjdk.java.net/~redestad/8186500/jdk.01/
>>> Still think testing for {null, Class, MethodHandle, MethodType} would cover more interesting corner
>>> cases.
>> Do you mean as constant arguments? And what should happen in each of these cases?
> As Remi says, should be equivalent to String.valueOf().
>
> null: should be converted to "null"
> {Class, MH, MT}: should be equivalent to Class::toString
>
> My concern is that we have to check the new code, e.g. Wrapper.* does not barf when we pass
> something non-primitive, non-String there.
>
> Thanks,
> -Aleksey
>
>