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
