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

[Aleksey Shipilev]

On 11/23/2015 12:36 PM, Ivan Gerasimov wrote:
> 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.

Yes, okay:
  http://cr.openjdk.java.net/~shade/8136500/webrev.04/

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

What sanity checks you had in mind? Because
java/lang/Integer/IntegerDecode.java fails, and it fails *in compiler*.
I tried to see what might be wrong, but failed.

The mere fact the debugging is hard for this code tells me that we are
much better off having cleaner code that handles MIN_VALUE separately.
If you still want MIN_VALUE to go away, then file a follow up, and try
to disentangle those weird failures ;)

Also, with this kind of change, AbstractStringBuilders would *still*
have the MIN_VALUE checks, *plus* all the additional handling logic in
stringSize and getChars, that will run for positive values. You can
explore removing that as well, but it makes sense only after Indify
String Concat lands and brings its own stringSize/getChar usages (and
tests!) on the table.


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

No, I think the perfect is the enemy of done here.

Thanks,
-Aleksey