Code (Pre-)Review for JEP 280: Indify String Concat

[Andrej Golovnin]

Hi Aleksey,

>>   http://cr.openjdk.java.net/~shade/8085796/webrev.jdk.04/

src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java

356         ARG,

The comma is not needed.

 548             // Mock the recipe to reuse the concat generator code
 549             StringBuilder sb = new StringBuilder();
 550             for (int c = 0; c < concatType.parameterCount(); c++) {
 551                 sb.append(TAG_ARG);
 552             }
 553             recipe = sb.toString();

This can be rewritten as:

char[] value = new char[concatType.parameterCount()];
Arrays.fill(value, TAG_ARG);
recipe = new String(value);

And when you add to the JavaLangAccess interface a new method
newStringUnsafe(byte[] value, byte coder), then you can even use the
package private constructor String(byte[], byte) to avoid copying of
bytes. But I'm not sure if it is worthwhile.

 558         int cCount = 0;
 559         int oCount = 0;
 560         for (int i = 0; i < recipe.length(); i++) {
 561             char c = recipe.charAt(i);
 562             if (c == TAG_CONST) cCount++;
 563             if (c == TAG_ARG)   oCount++;
 564         }

This loop is only needed, when the recipe is already known. When you
move the lines 558 and 559 before the line

 547         if (generateRecipe) {

then you can initialize oCount directly after the line 547:

oCount = concatType.parameterCount();

and move the for-loop (lines 560-564) into the else clause of the "if
(generateRecipe)" statement.

Btw. I would rename the variables cCount and oCount to constCount and argCount.

 601         Key key = new Key(mt, rec);
 602         MethodHandle mh = CACHE_ENABLE ? CACHE.get(key) : null;

If you rewrite this lines as:

 601         Key key = null;
 602         MethodHandle mh = CACHE_ENABLE ? CACHE.get(key = new
Key(mt, rec)) : null;

then you can avoid creation of Key objects when caching is disabled.

 669         SIZED_EXACT(true, true),

The comma is not needed.

 672         boolean sized, exact;

sized and exact should be final.

1619         private static Class<?> STRING_HELPER;
1620
1621         static {
1622             try {
1623                 STRING_HELPER =
Class.forName("java.lang.StringConcatHelper");
1624             } catch (ClassNotFoundException e) {
1625                 throw new AssertionError(e);
1626             }

Why STRING_HELPER is not final?

I have a small question to the Recipe class. Is it really better to
create elementsRev instead of using the old-school for-loop and
List.get(int) to loop over the list of RecipeElements in the reverse
order? If there is any reason, then could you please document it to
avoid stupid questions in the future? :-)

src/java.base/share/classes/java/lang/invoke/StringConcatException.java

  31     private static final long serialVersionUID = 292L + 9L;

I see that the pattern for serialVersionUID like this is already used
in four other classes (BootstrapMethodError, MethodType,
WrongMethodTypeException, LambdaConversioinException). Does it have
some special meaning? I'm just curious.

Best regards,
Andrej Golovnin