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

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

I'd like you to try to port my related RangeCheckMicroBenchmark
to string handling and hopefully demonstrate some measurable
performance improvement.

----

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);

----
it's => its

+     *              following values are referred in it's message:

----

badIndex might be a better name for "index" below.

+        int index;
+        if (begin < 0)
+            index = begin;

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

----

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;
+    }

----

We will need a significant merge once I commit
related changes.

----

Thanks,

Martin


On Sat, Mar 27, 2010 at 14:08, Ulf Zibis <Ulf.Zibis at gmx.de> wrote:
> Am 22.03.2010 23:27, schrieb Martin Buchholz:
>>
>> Ulf,
>>
>> I'd like to start a mq patch containing changes to
>> the String exception handling in the string classes.
>> Please provide me with a patch that uses the
>> blessed conventional names from Preconditions.java.
>>
>
> Here are my first patches for start.
> In the 2nd patch I did additional speed-ups, corrections and renamings.
>
> Please review.
>
> -Ulf
>
>
>> For the version that checks an offset and length for
>> containment within a larger sequence, I would prefer
>> the name "checkSubsequence", for example
>>
>> private static void checkSubsequence(int start, int len, int size)
>>
>> Please make sure that there are sufficient tests in
>> test/java/lang/String to ensure that you are not
>> inadvertently making changes to the exceptions thrown.
>>
>> I note that test/java/lang/String/{Exceptions,Supplementary}
>> do try to test exception handling, but do not appear to
>> test for the *exact* class of the exception thrown,
>> nor the detail message of the exception.
>> When those tests were written, compatibility was less important.
>>
>> Please adapt my
>> test/java/util/ArrayList/RangeCheckMicroBenchmark.java
>> to test string classes instead.
>> There is a good chance that you can demonstrate
>> a performance improvement on ordinary String operations!
>>
>> Thanks,
>>
>> Martin
>>
>>
>>
>