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

2017-08-24 Thread Aleksey Shipilev
On 08/24/2017 12:31 AM, Claes Redestad wrote: > http://cr.openjdk.java.net/~redestad/8186500/jdk.04/ Looks good! -Aleksey

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

2017-08-23 Thread Claes Redestad
On 2017-08-23 18:35, Aleksey Shipilev wrote: Sure, mind if I defer that to a future RFE, though?:-) Oh, c'mon, that should be a simple change:) And it makes the patch (that we would have to backport some day) more readable! Ok then: http://cr.openjdk.java.net/~redestad/8186500/jdk.04/ /C

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

2017-08-23 Thread Aleksey Shipilev
On 08/23/2017 06:31 PM, Claes Redestad wrote: > > > On 08/23/2017 06:31 PM, Aleksey Shipilev wrote: >> On 08/23/2017 06:26 PM, Claes Redestad wrote: >>> On 08/23/2017 06:08 PM, Aleksey Shipilev wrote: 332 Object value = Objects.requireNonNull(cnst); 333 if

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

2017-08-23 Thread Claes Redestad
On 08/23/2017 06:31 PM, Aleksey Shipilev wrote: On 08/23/2017 06:26 PM, Claes Redestad wrote: On 08/23/2017 06:08 PM, Aleksey Shipilev wrote: 332 Object value = Objects.requireNonNull(cnst); 333 if (!value.getClass().isPrimitive()) { 334 this.v

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

2017-08-23 Thread Aleksey Shipilev
On 08/23/2017 06:26 PM, Claes Redestad wrote: > On 08/23/2017 06:08 PM, Aleksey Shipilev wrote: >> 332 Object value = Objects.requireNonNull(cnst); >> 333 if (!value.getClass().isPrimitive()) { >> 334 this.value = String.valueOf(cnst); >> 335

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

2017-08-23 Thread Claes Redestad
On 08/23/2017 06:08 PM, Aleksey Shipilev wrote: On 08/23/2017 01:52 PM, Claes Redestad wrote: 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

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

2017-08-23 Thread Aleksey Shipilev
On 08/23/2017 01:52 PM, Claes Redestad wrote: > 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 >

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

2017-08-23 Thread 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 toStrin

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

2017-08-22 Thread Aleksey Shipilev
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 arg

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

2017-08-22 Thread Remi Forax
t; Envoyé: Mardi 22 Août 2017 19:10:29 > Objet: Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws > AssertionError when recipe contains > non-String constants > On 2017-08-22 18:34, Aleksey Shipilev wrote: >>> http://cr.openjdk.java.net/~redestad/8186500/jdk.

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

2017-08-22 Thread Claes Redestad
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? /

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

2017-08-22 Thread Aleksey Shipilev
On 08/22/2017 04:53 PM, Claes Redestad wrote: > Extending the existing StringConcatFactoryInvariants test we can easily get > most basic > combinations covered, along with some basic verification. > > This uncovered what appears to be a similar issue in the BC_SB SCF strategy, > which can be > d

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

2017-08-22 Thread Claes Redestad
Extending the existing StringConcatFactoryInvariants test we can easily get most basic combinations covered, along with some basic verification. This uncovered what appears to be a similar issue in the BC_SB SCF strategy, which can be dealt with by coercing the possibly non-String constant to S

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

2017-08-21 Thread Aleksey Shipilev
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 wrote: >>> a trivial test[1] invoking the StringConcatFactory.makeConcatWithConstants >>> fails >>> when providing an Integer as a constant, which appears to b

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

2017-08-21 Thread Aleksey Shipilev
On 08/21/2017 08:06 PM, Paul Sandoz wrote: >> On 21 Aug 2017, at 07:48, Claes Redestad 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 c

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

2017-08-21 Thread Paul Sandoz
> On 21 Aug 2017, at 07:48, Claes Redestad wrote: > > Hi, > > 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 looki