Refactor String's exception handling

[Martin Buchholz]

On Tue, Mar 30, 2010 at 04:46, Ulf Zibis <Ulf.Zibis at gmx.de> wrote:
> Am 30.03.2010 00:17, schrieb Martin Buchholz:
>
> Hi Ulf,
>
> I will sponsor your initiative to refactor the exception handling.
>
> Before this can go in, we should have just the exception handling
> changes contained in one patch, since it is such a big change.
>
>
> You mean, that I had "surreptitiously" included some beautification, even in
> the first patch?
> Yes, often I can't resist, hit me. Example:
> It seems, that someone before had tried to standardize the this-triple in
> String's constructors. Looking closer, you can see, that they slightly
> differ, so for my taste it looked best, ordering them in the member
> variables order, having the real value at first.

Yes, it's a tough question as to how finely to split changes.
The overhead of creating separate changes (must have a bug ID)
is unfortunately higher than we'd like.

That said, there are big advantages of separating out
purely cosmetic large changes.  E.g. we can verify
that the generated bytecode is identical.
This becomes much more important for pervasive
mechanical changes, like changing @exception => @throws.

> On the other hand, I think it's too much overhead, to manage separate bugs
> for such beautifications.
> What you think is a reasonable threshold for such on-the-fly
> beautifications?

> You seem to like how I merged the different variations into one central
> standard behaviour. Is that valid for AbstractStringBuilder too?
> I think it best matches to current behavior.
> Exception message refers to ...
> 1. <begin>, if begin itself is invalid referring to 0 and srcLen
> 2. <end>, if end itself is invalid referring to 0 and srcLen
> 3. <end-begin>, if end is invalid in combination with given begin
> Alternative:
> 2+3. <end>, if end is invalid referring to 0 and srcLen or in combination
> with given begin

Better detail messages are slightly incompatible,
but helpful for most users.  Should we switch?
It depends on how much we value compatibility.
Probably the JDK culture is still too conservative.

> ----
> Run at least the following tests
> (below is how I test this code myself)
>
> /home/martinrb/jct-tools/3.2.2_03/linux/bin/jtreg -v:nopass,fail
> -vmoption:-enablesystemassertions -automatic "-k:\!ignore"
> -testjdk:/usr/local/google/home/martin/ws/upstream/build/linux-amd64
> test/sun/nio/cs test/java/nio/charset test/java/lang/StringCoding
> test/java/lang/StringBuilder test/java/lang/StringBuffer
> test/java/lang/String test/java/lang/Appendable
>
>
> Unfortunately I still haven't managed to even partly build a patched JDK on
> my Windows notebook.

It's fine to run javac + use -Xbootclasspath.

> - CygWin crashes from too big work, e.g webrev on more than ~20 files.
> - Very few support on <nb-projects-dev at openjdk.java.net> mailing list.
> - I'm wondering, that there is so few collaboration between NetBeans and JDK
> developers in same software company.
>
> So as workaround, I'm fine with running my patches via -Xbootclasspath in
> NetBeans IDE.
> So running jtreg tests I don't know how.

You should really learn how to run JDK tests.
JDK development on Linux is easier than development on Windows,
but it certainly should be possible on Windows.
Recent versions of jtreg are probably easier to run on Windows.
http://openjdk.java.net/jtreg/index.html

> I exclusively had written my test using JUnit, because there is a beautiful
> support from NetBeans.
> I remember, there was a email from Mark Reinold some months ago, that JUnit
> tests are too supported by jtreg from now.

I would be interested in that as well.

> Maybe you have some suggestions to me.
>
> ----
>
> I think returning len below is too confusing.
> Just make the return type void.
>
> +    int checkPositionIndex(int index) {
> +        int len = count; // not sure, if JIT recognizes that it's final ?
> +        checkPositionIndex(len, index);
> +        return len;
> +    }
>
>
> Returning the len is to prevent from 2 times slowly loading the member
> variable into local register/variable.
> From performance side I think, we only have to choices. Using the return
> trick or dropping those convenient methods at all.
> The latter would be faster for the interpreter and/or non inlined case.

In core libraries we often make engineering decisions to use
trickier or more verbose code for the sake of performance,
but I think this is going over the line.
I think you can rely on inlining of such small, always called,
methods like checkPositionIndex.

> ----
>
> We will need a significant merge once I commit
> related changes.
>
>
> Maybe we could announce this on this list, so other's could decide, if they
> hurry to commit there changes before, or have to do there own merge later.

I run into lots of merge conflicts, but always with my own
changes!  I don't think we have a lot of contention.

Martin