----- Mail original -----
> De: "Claes Redestad" <claes.redes...@oracle.com>
> À: "Remi Forax" <fo...@univ-mlv.fr>
> Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net>
> Envoyé: Vendredi 29 Mai 2020 20:45:26
> Objet: Re: RFR: 8246152: Improve String concat bootstrapping
> On 2020-05-29 19:32, fo...@univ-mlv.fr wrote:
>> ----- Mail original -----
>>> De: "Claes Redestad" <claes.redes...@oracle.com>
>>> À: "Remi Forax" <fo...@univ-mlv.fr>
>>> Cc: "core-libs-dev" <core-libs-dev@openjdk.java.net>
>>> Envoyé: Vendredi 29 Mai 2020 18:47:03
>>> Objet: Re: RFR: 8246152: Improve String concat bootstrapping
>>
>>> Hi Rémi,
>>>
>>> thanks for looking at this and suggesting some improvements!
>>>
>>> On 2020-05-29 17:51, Remi Forax wrote:
>>>> Hi Claes,
>>>> For the code below the comment "Mock the recipe to reuse the concat
>>>> generator
>>>> code",
>>>> i believe you can use String.repeat() that was introduced recently.
>>>
>>> Sure,
>>>
>>>>
>>>> The code that parse the the receipe can be in its own method to make the
>>>> code
>>>> more readable,
>>>> this method returns the list and use the StringBuilder internally.
>>>
>>> sure,
>>>
>>>>
>>>> In generateMHInlineCopy,
>>>> element.get(0) and element.get(1) should be stored in local variables
>>>> after
>>>> "elements.size() == 2"
>>>> will make the code more readable
>>>
>>> sure,
>>>
>>>>
>>>> In simpleConcat(),
>>>> should use a local variable 'mh', like the newString or
>>>> newArrayWithSuffix
>>>
>>> yes!
>>>
>>> http://cr.openjdk.java.net/~redestad/8246152/open.01/
>>
>>
>> I don't think you need to check MAX_INDY_CONCAT_ARG_SLOTS anymore, given that
>> now indy is based on invokeWithArguments instead of invoke.
>
> Not sure what's changed, but let's leave this outside the scope of this
> RFE?
This is https://bugs.openjdk.java.net/browse/JDK-8186469
Agree that it can be changed later.
>
>>
>> And i think you should keep the null checks of constants upfront to not
>> change
>> the semantics (you are allowing constant not referenced by a recipe to be
>> null).
>
> If anyone provides more constants than we use (including null values)
> we'll fail the cCount != constants.length check later and throw a
> StringConcatException anyway. So no real, semantic difference apart from
> a change in what the exception message will say, which I think is OK,
> no?
>
>>
>> The catch should be:
>> catch (Error | RuntimeException e) {
>> if you want the StringConcatException to be propagated
>
> StringConcatException is a regular Exception and is not thrown from
> generateMHInlineCopy.
in that case, you should fix the comment ?
>
>>
>> all the other changes looks good.
>
> Thanks!
>
> /Claes
Rémi
>
>>
>>>
>>> Re-running tier1
>>>
>>> /Claes
>>
>> Rémi
