review request for 6798511/6860431: Include functionality of Surrogate in Character

[Martin Buchholz]

On Thu, Mar 25, 2010 at 08:26, Ulf Zibis <Ulf.Zibis at gmx.de> wrote:
> Am 22.03.2010 23:03, schrieb Martin Buchholz:
>>
>> On Mon, Mar 22, 2010 at 07:34, Ulf Zibis<Ulf.Zibis at gmx.de>  wrote:
>>
>>>
>>> Am 21.03.2010 17:16, schrieb Martin Buchholz:
>>>
>>
>>
>>>>
>>>> There is a debate about whether to reuse existing exception classes
>>>> or to throw class-specific subclasses.  IMO, IOOBE is a sufficiently
>>>> expressive
>>>> exception that I might have used just that, with expressive detail
>>>> messages.
>>>>
>>>>
>>>
>>> I'm with you. Especially StringIndexOutOfBoundsException appears as
>>> superfluous sugar to me. But we have it in the docs, so there is no way
>>> to
>>> get rid of it.
>>> What do you think about to refactor most IOOBEs in String related classes
>>> to
>>> SIOOBEs? It would stay compatible to old Software, which still catches
>>> IOOBEs, but would look more straight, tidy and clean and fix the below
>>> mentioned bug.
>>>
>>
>> Every change is an incompatible change, with a risk/benefit tradeoff.
>>
>> IMO there is no change to the exceptions thrown, or declared to be thrown,
>> or to their detail messages, in the string classes that is worth the risk
>> of incompatible change.
>>
>
> Is somewhat reasonable, but what's the win of those "creative" variations on
> exception messages _and_ types in AbstractStringBuilder? :
> throw new StringIndexOutOfBoundsException();
> throw new StringIndexOutOfBoundsException(index);
> throw new StringIndexOutOfBoundsException(start);
> throw new StringIndexOutOfBoundsException("start > length()");
> throw new StringIndexOutOfBoundsException("start > end");
> throw new StringIndexOutOfBoundsException(end - start);
> throw new StringIndexOutOfBoundsException(srcEnd);
> throw new StringIndexOutOfBoundsException("srcBegin > srcEnd");
> throw new IndexOutOfBoundsException();
> throw new IndexOutOfBoundsException("start " + start + ", end " + end + ",
> s.length() " + s.length());
> throw new IndexOutOfBoundsException("dstOffset "+dstOffset);

It's a sad situation.  It's certain someone is stupid enough
to have written a program that depends on the details above,
and the question is whether the improvement is worthwhile.
A measurable performance improvement will make your case
much stronger.

>> (with the exception of when the implementation contradicts the spec,
>> which is worth fixing)
>>
>
> #insert(int, char[], in, int), uses System.arraycopy().
> If capacity doesn't suffice, it would throw an IOOBE, not SIOOBE
>
> #insert(int, CharSequence) states:
>     * @throws     IndexOutOfBoundsException  if the offset is invalid.
> but (1) in fact throws SIOOBE in described case, if CharSequence is of
> String.
> and (2) additionally throws IOOBE in case of capacity overflow, which is not
> mentioned.
>
> #insert(...) methods mix between (int index, ...) and (int dstIndex, ...)
> without any reason.
>
> #substring(int) could be faster not using substring(int, int) detailed
> bounds checking.
>
> #subSequence(int, int) in fact throws SIOOBE instead IOOBE.
>
> #appendCodePoint(int) could throw AIOOBE, similar to many other append
> methods, capacity overflow behaviour is not documented.
>
> I stop here ... ;-)

Several of us have been here,
wanting to improve these minor blemishes,
and eventually deciding to put our effort elsewhere.

I am in favor of at least fixing the detail messages and making the
argument names more regular, but I you'll have to get the support
of others as well.  Sherman?

Martin