RFR (S) 8136500: Integer/Long getChars and stringSize should be more idiomatic

[Ivan Gerasimov]

Hi Aleksey!

I think the code looks much better now!

A couple of concerns, though:
1)
Wouldn't it be clearer to have "(byte)('0' + q);" instead of "(byte)(48 
+ q); // 48 is ASCII '0'"?
I see that '0' constant is used in arithmetic in some other places.

2)
What still annoys me is the necessity to handle the MIN_VALUE separately.
It seems to be possible to generalize the stringSize() and getChars() to 
handle this edge case as well, keeping the amount of arithmetic the same.

Under the link please find a patch, which does that for Integer.
http://cr.openjdk.java.net/~igerasim/8136500/8136500-addition.patch

The patch was created on top of the changes from your latest webrev.
The sanity test passed, though it might be necessary to add some more 
testing.

Do you think it's worth it to reorganize the code this way?

Sincerely yours,
Ivan


On 22.11.2015 22:59, Aleksey Shipilev wrote:
> On 11/22/2015 01:55 AM, Ivan Gerasimov wrote:
>>>> Second, I think, the code of stringSize() might be better inlined, if it
>>>> was used only once.
>>> No-no! That's a helpful little fella. Not-yet-integrated Indify String
>>> Concat uses Integer/Long.stringSize.
>> I didn't really propose to manually inline the stringSize() into the
>> calling code.
>> What I was trying to say was that if we replace the code "int size = (i
>> < 0) ? stringSize(-i) + 1 : stringSize(i)", which calls stringSize()
>> twice, with something, which calls it only once, it might be easier for
>> compiler to inline it.
>>
>> Sorry, I didn't make it clear :)
> Ah, that "only once" confused me. Yes, that makes sense. In fact, I
> think we should just soak the negative check into the stringSize, given
> that all the usages are doing the checks externally. This exposes the
> MIN_VALUE checks back, but it seems a fair trade for cleaner code; the
> performance is still on par with previous revisions. And, this also
> caters for AbstractStringBuilders, as you and Fabian wanted.
>
> See:
>   http://cr.openjdk.java.net/~shade/8136500/webrev.03/
>
> Thanks,
> -Aleksey
>