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

[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
> our earliest convenience, which might even be in the RecipeElement construction:

Yes, I don't see why not.

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

Hm!

 *) Seems that due to cnst being an (autoboxed) reference, isPrimitive is always false on this path?

 332             Object value = Objects.requireNonNull(cnst);
 333             if (!value.getClass().isPrimitive()) {
 334                 this.value = String.valueOf(cnst);
 335             } else {
 336                 this.value = value;
 337             }

...so that value is always String after this?
...and that avoids String conversion on RecipeElement.getValue() paths completely?

 *) If not, these are replaceable with String.valueOf:

 937                             String s = (cnst != null) ? cnst.toString() : "null";
...
 998                             String s = (cnst != null) ? cnst.toString() : "null";


> 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).

Ah, good! I was not completely reckless with "null" checking after all!

Thanks,
-Aleksey