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.
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?
I'd like you to try to port my related RangeCheckMicroBenchmark
to string handling and hopefully demonstrate some measurable
performance improvement.
That would be great. :-)
----
In the code below, I think some if's need to be changed to "else if"s.
(but don't just fix it - make sure we have a failing test with your
current code (you do run the regression tests religiously, right?))
+ static void checkPositionIndexes(int srcLen, int begin, int end) {
+ assert (srcLen>= 0);
+ int index;
+ if (begin< 0)
+ index = begin;
+ if (end> srcLen)
+ index = begin>srcLen ? begin:end-begin;
+ if (end< begin)
+ index = begin>srcLen ? begin : end<0 ? end : end-begin;
+ else
+ return;
+ throw new StringIndexOutOfBoundsException(index);
Good catch. The throws, I had replaced, had implicated the elses before.
In Google code style it would have been: if (begin < 0 || end > srcLen
|| end < begin)
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
The alternative may be easier to track for the developers, but less
compatible with current behaviour, and a likly negative value speaks
kinda for itself.
In the checkSubsequence(..., offset, count) case, unfortunately there is
a good chance to have positive values as result of offset+count.
----
it's => its
+ * following values are referred in it's message:
Yes.
----
badIndex might be a better name for "index" below.
+ int index;
+ if (begin< 0)
+ index = begin;
Very good idea!
----
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.
- CygWin crashes from too big work, e.g webrev on more than ~20 files.
- Very few support on <[email protected]> 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.
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.
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.
----
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.
-Ulf
