You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@harmony.apache.org by je...@apache.org on 2009/11/12 04:25:53 UTC
svn commit: r835212 -
/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java
Author: jessewilson
Date: Thu Nov 12 03:25:52 2009
New Revision: 835212
URL: http://svn.apache.org/viewvc?rev=835212&view=rev
Log:
Cleanup and bugfix BufferedReader.
This change includes the following functional changes:
- changing read to not clear the mark upon reading EOF. The previous behaviour was incorrect.
- changing read(char[], int, int) to use the 'read directly from the source stream' shortcut when the mark has exceeded its limit. Previously we took the shortcut only when the mark was unset.
And these nonfunctional changes:
- rewrote read(char[], int, int). The new revision contains only one call to 'System.arrayCopy' and the related bookkeeping. Previously there was one call before the loop, and another call in the loop.
- renamed markpos to mark
- renamed marklimit to markLimit
- renamed count to end (it isn't a count, it's a position)
- renamed fillbuf() to fillBuf()
- simplified conditions that used >= when > was impossible
- reducing the number of field reads where convenient
This tidy up is intended to prepare BufferedReader for some further bugfixing. We've seen some bugs reported against readLine() and I found it quite frustrating to work on the code when the names were wrong (ie. count) or of a foreign style (such as fillbuf()). I also attempted to document what the heck was going on in the more sophiticated methods.
Modified:
harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java
Modified: harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java
URL: http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java?rev=835212&r1=835211&r2=835212&view=diff
==============================================================================
--- harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java (original)
+++ harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java Thu Nov 12 03:25:52 2009
@@ -40,15 +40,35 @@
private Reader in;
+ /**
+ * The characters that can be read and refilled in bulk. We maintain three
+ * indices into this buffer:<pre>
+ * { X X X X X X X X X X X X - - }
+ * ^ ^ ^
+ * | | |
+ * mark pos end</pre>
+ * Pos points to the next readable character. End is one greater than the
+ * last readable character. When {@code pos == end}, the buffer is empty and
+ * must be {@link #fillBuf() filled} before characters can be read.
+ *
+ * <p>Mark is the value pos will be set to on calls to {@link #reset}. Its
+ * value is in the range {@code [0...pos]}. If the mark is {@code -1}, the
+ * buffer cannot be reset.
+ *
+ * <p>MarkLimit limits the distance between the mark and the pos. When this
+ * limit is exceeded, {@link #reset} is permitted (but not required) to
+ * throw an exception. For shorter distances, {@link #reset} shall not throw
+ * (unless the reader is closed).
+ */
private char[] buf;
- private int marklimit = -1;
+ private int pos;
- private int count;
+ private int end;
- private int markpos = -1;
+ private int mark = -1;
- private int pos;
+ private int markLimit = -1;
/**
* Constructs a new BufferedReader on the Reader {@code in}. The
@@ -101,36 +121,50 @@
}
}
- private int fillbuf() throws IOException {
- if (markpos == -1 || (pos - markpos >= marklimit)) {
- /* Mark position not set or exceeded readlimit */
+ /**
+ * Populates the buffer with data. It is an error to call this method when
+ * the buffer still contains data; ie. if {@code pos < end}.
+ *
+ * @return the number of bytes read into the buffer, or -1 if the end of the
+ * source stream has been reached.
+ */
+ private int fillBuf() throws IOException {
+ // assert(pos == end);
+
+ if (mark == -1 || (pos - mark >= markLimit)) {
+ /* mark isn't set or has exceeded its limit. use the whole buffer */
int result = in.read(buf, 0, buf.length);
if (result > 0) {
- markpos = -1;
+ mark = -1;
pos = 0;
- count = result == -1 ? 0 : result;
+ end = result;
}
return result;
}
- if (markpos == 0 && marklimit > buf.length) {
- /* Increase buffer size to accommodate the readlimit */
+
+ if (mark == 0 && markLimit > buf.length) {
+ /* the only way to make room when mark=0 is by growing the buffer */
int newLength = buf.length * 2;
- if (newLength > marklimit) {
- newLength = marklimit;
+ if (newLength > markLimit) {
+ newLength = markLimit;
}
char[] newbuf = new char[newLength];
System.arraycopy(buf, 0, newbuf, 0, buf.length);
buf = newbuf;
- } else if (markpos > 0) {
- System.arraycopy(buf, markpos, buf, 0, buf.length - markpos);
+ } else if (mark > 0) {
+ /* make room by shifting the buffered data to left mark positions */
+ System.arraycopy(buf, mark, buf, 0, buf.length - mark);
+ pos -= mark;
+ end -= mark;
+ mark = 0;
}
/* Set the new position and mark position */
- pos -= markpos;
- count = markpos = 0;
- int charsread = in.read(buf, pos, buf.length - pos);
- count = charsread == -1 ? pos : pos + charsread;
- return charsread;
+ int count = in.read(buf, pos, buf.length - pos);
+ if (count != -1) {
+ end += count;
+ }
+ return count;
}
/**
@@ -144,32 +178,32 @@
}
/**
- * Sets a mark position in this reader. The parameter {@code readlimit}
+ * Sets a mark position in this reader. The parameter {@code markLimit}
* indicates how many characters can be read before the mark is invalidated.
* Calling {@code reset()} will reposition the reader back to the marked
- * position if {@code readlimit} has not been surpassed.
+ * position if {@code markLimit} has not been surpassed.
*
- * @param readlimit
+ * @param markLimit
* the number of characters that can be read before the mark is
* invalidated.
* @throws IllegalArgumentException
- * if {@code readlimit < 0}.
+ * if {@code markLimit < 0}.
* @throws IOException
* if an error occurs while setting a mark in this reader.
* @see #markSupported()
* @see #reset()
*/
@Override
- public void mark(int readlimit) throws IOException {
- if (readlimit < 0) {
+ public void mark(int markLimit) throws IOException {
+ if (markLimit < 0) {
throw new IllegalArgumentException();
}
synchronized (lock) {
if (isClosed()) {
throw new IOException(Msg.getString("K005b")); //$NON-NLS-1$
}
- marklimit = readlimit;
- markpos = pos;
+ this.markLimit = markLimit;
+ mark = pos;
}
}
@@ -205,10 +239,9 @@
throw new IOException(Msg.getString("K005b")); //$NON-NLS-1$
}
/* Are there buffered characters available? */
- if (pos < count || fillbuf() != -1) {
+ if (pos < end || fillBuf() != -1) {
return buf[pos++];
}
- markpos = -1;
return -1;
}
}
@@ -245,55 +278,64 @@
if (isClosed()) {
throw new IOException(Msg.getString("K005b")); //$NON-NLS-1$
}
- if (offset < 0 || offset > buffer.length - length || length < 0) {
- throw new IndexOutOfBoundsException();
+ if (buffer == null) {
+ throw new NullPointerException(Msg.getString("K0047")); //$NON-NLS-1$
}
- if (length == 0) {
- return 0;
- }
- int required;
- if (pos < count) {
- /* There are bytes available in the buffer. */
- int copylength = count - pos >= length ? length : count - pos;
- System.arraycopy(buf, pos, buffer, offset, copylength);
- pos += copylength;
- if (copylength == length || !in.ready()) {
- return copylength;
- }
- offset += copylength;
- required = length - copylength;
- } else {
- required = length;
+ if ((offset | length) < 0 || offset > buffer.length - length) {
+ throw new IndexOutOfBoundsException(Msg.getString("K002f")); //$NON-NLS-1$
}
+ int outstanding = length;
+ while (outstanding > 0) {
- while (true) {
- int read;
/*
- * If we're not marked and the required size is greater than the
- * buffer, simply read the bytes directly bypassing the buffer.
+ * If there are bytes in the buffer, grab those first.
*/
- if (markpos == -1 && required >= buf.length) {
- read = in.read(buffer, offset, required);
- if (read == -1) {
- return required == length ? -1 : length - required;
- }
- } else {
- if (fillbuf() == -1) {
- return required == length ? -1 : length - required;
+ int available = end - pos;
+ if (available > 0) {
+ int count = available >= outstanding ? outstanding : available;
+ System.arraycopy(buf, pos, buffer, offset, count);
+ pos += count;
+ offset += count;
+ outstanding -= count;
+ }
+
+ /*
+ * Before attempting to read from the underlying stream, make
+ * sure we really, really want to. We won't bother if we're
+ * done, or if we've already got some bytes and reading from the
+ * underlying stream would block.
+ */
+ if (outstanding == 0 || (outstanding < length && !in.ready())) {
+ break;
+ }
+
+ // assert(pos == end);
+
+ /*
+ * If we're unmarked and the requested size is greater than our
+ * buffer, read the bytes directly into the caller's buffer. We
+ * don't read into smaller buffers because that could result in
+ * a many reads.
+ */
+ if ((mark == -1 || (pos - mark >= markLimit))
+ && outstanding >= buf.length) {
+ int count = in.read(buffer, offset, outstanding);
+ if (count > 0) {
+ offset += count;
+ outstanding -= count;
+ mark = -1;
}
- read = count - pos >= required ? required : count - pos;
- System.arraycopy(buf, pos, buffer, offset, read);
- pos += read;
- }
- required -= read;
- if (required == 0) {
- return length;
+
+ break; // assume the source stream gave us all that it could
}
- if (!in.ready()) {
- return length - required;
+
+ if (fillBuf() == -1) {
+ break; // source is exhausted
}
- offset += read;
}
+
+ int count = length - outstanding;
+ return (count > 0 || count == length) ? count : -1;
}
}
@@ -313,11 +355,11 @@
if (isClosed()) {
throw new IOException(Msg.getString("K005b")); //$NON-NLS-1$
}
- /* Are there buffered characters available? */
- if ((pos >= count) && (fillbuf() == -1)) {
+ /* has the underlying stream been exhausted? */
+ if (pos == end && fillBuf() == -1) {
return null;
}
- for (int charPos = pos; charPos < count; charPos++) {
+ for (int charPos = pos; charPos < end; charPos++) {
char ch = buf[charPos];
if (ch > '\r') {
continue;
@@ -329,7 +371,7 @@
} else if (ch == '\r') {
String res = new String(buf, pos, charPos - pos);
pos = charPos + 1;
- if (((pos < count) || (fillbuf() != -1))
+ if (((pos < end) || (fillBuf() != -1))
&& (buf[pos] == '\n')) {
pos++;
}
@@ -341,27 +383,28 @@
StringBuilder result = new StringBuilder(80);
/* Typical Line Length */
- result.append(buf, pos, count - pos);
- pos = count;
+ result.append(buf, pos, end - pos);
while (true) {
+ pos = end;
+
/* Are there buffered characters available? */
- if (pos >= count) {
- if (eol == '\n') {
- return result.toString();
- }
- // attempt to fill buffer
- if (fillbuf() == -1) {
- // characters or null.
- return result.length() > 0 || eol != '\0' ? result
- .toString() : null;
- }
+ if (eol == '\n') {
+ return result.toString();
+ }
+ // attempt to fill buffer
+ if (fillBuf() == -1) {
+ // characters or null.
+ return result.length() > 0 || eol != '\0'
+ ? result.toString()
+ : null;
}
- for (int charPos = pos; charPos < count; charPos++) {
+ for (int charPos = pos; charPos < end; charPos++) {
+ char c = buf[charPos];
if (eol == '\0') {
- if ((buf[charPos] == '\n' || buf[charPos] == '\r')) {
- eol = buf[charPos];
+ if ((c == '\n' || c == '\r')) {
+ eol = c;
}
- } else if (eol == '\r' && (buf[charPos] == '\n')) {
+ } else if (eol == '\r' && c == '\n') {
if (charPos > pos) {
result.append(buf, pos, charPos - pos - 1);
}
@@ -376,11 +419,10 @@
}
}
if (eol == '\0') {
- result.append(buf, pos, count - pos);
+ result.append(buf, pos, end - pos);
} else {
- result.append(buf, pos, count - pos - 1);
+ result.append(buf, pos, end - pos - 1);
}
- pos = count;
}
}
@@ -388,7 +430,7 @@
/**
* Indicates whether this reader is ready to be read without blocking.
- *
+ *
* @return {@code true} if this reader will not block when {@code read} is
* called, {@code false} if unknown or blocking will occur.
* @throws IOException
@@ -403,7 +445,7 @@
if (isClosed()) {
throw new IOException(Msg.getString("K005b")); //$NON-NLS-1$
}
- return ((count - pos) > 0) || in.ready();
+ return ((end - pos) > 0) || in.ready();
}
}
@@ -423,17 +465,17 @@
if (isClosed()) {
throw new IOException(Msg.getString("K005b")); //$NON-NLS-1$
}
- if (markpos == -1) {
+ if (mark == -1) {
throw new IOException(Msg.getString("K005c")); //$NON-NLS-1$
}
- pos = markpos;
+ pos = mark;
}
}
/**
* Skips {@code amount} characters in this reader. Subsequent
* {@code read()}s will not return these characters unless {@code reset()}
- * is used. Skipping characters may invalidate a mark if {@code readlimit}
+ * is used. Skipping characters may invalidate a mark if {@code markLimit}
* is surpassed.
*
* @param amount
@@ -459,24 +501,24 @@
if (amount < 1) {
return 0;
}
- if (count - pos >= amount) {
+ if (end - pos >= amount) {
pos += amount;
return amount;
}
- long read = count - pos;
- pos = count;
+ long read = end - pos;
+ pos = end;
while (read < amount) {
- if (fillbuf() == -1) {
+ if (fillBuf() == -1) {
return read;
}
- if (count - pos >= amount - read) {
+ if (end - pos >= amount - read) {
pos += amount - read;
return amount;
}
// Couldn't get all the characters, skip what we read
- read += (count - pos);
- pos = count;
+ read += (end - pos);
+ pos = end;
}
return amount;
}
Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java
Posted by Regis <xu...@gmail.com>.
Alexei Fedotov wrote:
> Funny. We don't have a requirement that a committer have to test the
> code before committing it here [1].
>
> [1] http://wiki.apache.org/harmony/NewCommitter
>
>
This page just point out the steps to configure develop tools for a new
committer. Testing code is what *every* contributor must do before contributing
code to community.
--
Best Regards,
Regis.
Re: [testing] pre-commit testing (was: Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java)
Posted by Mark Hindess <ma...@googlemail.com>.
In message <4B...@gmail.com>, Tim Ellison writes:
>
> On 17/Nov/2009 03:50, Nathan Beyer wrote:
> > 2009/11/16 Alexei Fedotov <al...@gmail.com>:
> >> Funny. We don't have a requirement that a committer have to test the
> >> code before committing it here [1].
> >
> > I suppose some things are just assumed.
> >
> >> [1] http://wiki.apache.org/harmony/NewCommitter
>
> Of course, we are all grown-ups. Mistakes happen.
>
> How close are we to being able to run all the tests via Hudson?
> I recall that there was discussion and work on packaging the tests so
> they can be run from the HDK. I don't recall it being declared 'done'?
"ant -f /path/to/hdk/build/test/build.xml" should work but there are
a few tests that fail when run this way (which is a bit odd since the
tests effectively run from there when you run "ant test" too. However,
I see consistently broken tests anyway so perhaps we should just start
running all the tests this way *and* investigate all of the failures
then start being stricter about keeping them working (or excluded).
Regards,
Mark.
Re: [testing] pre-commit testing (was: Re: svn commit: r835212 -
/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java)
Posted by Alexei Fedotov <al...@gmail.com>.
Ok, done.
On Wed, Nov 18, 2009 at 6:11 AM, Nathan Beyer <nb...@gmail.com> wrote:
> 2009/11/17 Alexei Fedotov <al...@gmail.com>:
>> I would rather call this "misunderstanding".
>>
>> Jesse run one set of tests, Tim expected another set. One way to
>
> If you think it should be documented on the wiki, go ahead and do it -
> it's open and intended to be less formal than the proper web site.
>
> -Nathan
>
>> correct the problem is to agree that committers run a specific minimal
>> set of tests, for example, on one platform. I thought we had such
>> agreements documented, but failed to find a proper pointer. It seems
>> to me now that instead of formal pre-commit rules we had Stepan
>> Mishura who guided us informally.
>>
>>
>>
>>
>> On Tue, Nov 17, 2009 at 6:38 PM, Tim Ellison <t....@gmail.com> wrote:
>>> On 17/Nov/2009 03:50, Nathan Beyer wrote:
>>>> 2009/11/16 Alexei Fedotov <al...@gmail.com>:
>>>>> Funny. We don't have a requirement that a committer have to test the
>>>>> code before committing it here [1].
>>>>
>>>> I suppose some things are just assumed.
>>>>
>>>>> [1] http://wiki.apache.org/harmony/NewCommitter
>>>
>>> Of course, we are all grown-ups. Mistakes happen.
>>>
>>> How close are we to being able to run all the tests via Hudson?
>>> I recall that there was discussion and work on packaging the tests so
>>> they can be run from the HDK. I don't recall it being declared 'done'?
>>>
>>> Regards,
>>> Tim
>>>
>>
>>
>>
>> --
>> With best regards / с наилучшими пожеланиями,
>> Alexei Fedotov / Алексей Федотов,
>> http://www.telecom-express.ru/
>> http://harmony.apache.org/
>> http://www.expressaas.com/
>> http://openmeetings.googlecode.com/
>>
>
--
With best regards / с наилучшими пожеланиями,
Alexei Fedotov / Алексей Федотов,
http://www.telecom-express.ru/
http://harmony.apache.org/
http://www.expressaas.com/
http://openmeetings.googlecode.com/
Re: [testing] pre-commit testing (was: Re: svn commit: r835212 -
/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java)
Posted by Nathan Beyer <nb...@gmail.com>.
2009/11/17 Alexei Fedotov <al...@gmail.com>:
> I would rather call this "misunderstanding".
>
> Jesse run one set of tests, Tim expected another set. One way to
If you think it should be documented on the wiki, go ahead and do it -
it's open and intended to be less formal than the proper web site.
-Nathan
> correct the problem is to agree that committers run a specific minimal
> set of tests, for example, on one platform. I thought we had such
> agreements documented, but failed to find a proper pointer. It seems
> to me now that instead of formal pre-commit rules we had Stepan
> Mishura who guided us informally.
>
>
>
>
> On Tue, Nov 17, 2009 at 6:38 PM, Tim Ellison <t....@gmail.com> wrote:
>> On 17/Nov/2009 03:50, Nathan Beyer wrote:
>>> 2009/11/16 Alexei Fedotov <al...@gmail.com>:
>>>> Funny. We don't have a requirement that a committer have to test the
>>>> code before committing it here [1].
>>>
>>> I suppose some things are just assumed.
>>>
>>>> [1] http://wiki.apache.org/harmony/NewCommitter
>>
>> Of course, we are all grown-ups. Mistakes happen.
>>
>> How close are we to being able to run all the tests via Hudson?
>> I recall that there was discussion and work on packaging the tests so
>> they can be run from the HDK. I don't recall it being declared 'done'?
>>
>> Regards,
>> Tim
>>
>
>
>
> --
> With best regards / с наилучшими пожеланиями,
> Alexei Fedotov / Алексей Федотов,
> http://www.telecom-express.ru/
> http://harmony.apache.org/
> http://www.expressaas.com/
> http://openmeetings.googlecode.com/
>
Re: [testing] pre-commit testing (was: Re: svn commit: r835212 -
/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java)
Posted by Alexei Fedotov <al...@gmail.com>.
I would rather call this "misunderstanding".
Jesse run one set of tests, Tim expected another set. One way to
correct the problem is to agree that committers run a specific minimal
set of tests, for example, on one platform. I thought we had such
agreements documented, but failed to find a proper pointer. It seems
to me now that instead of formal pre-commit rules we had Stepan
Mishura who guided us informally.
On Tue, Nov 17, 2009 at 6:38 PM, Tim Ellison <t....@gmail.com> wrote:
> On 17/Nov/2009 03:50, Nathan Beyer wrote:
>> 2009/11/16 Alexei Fedotov <al...@gmail.com>:
>>> Funny. We don't have a requirement that a committer have to test the
>>> code before committing it here [1].
>>
>> I suppose some things are just assumed.
>>
>>> [1] http://wiki.apache.org/harmony/NewCommitter
>
> Of course, we are all grown-ups. Mistakes happen.
>
> How close are we to being able to run all the tests via Hudson?
> I recall that there was discussion and work on packaging the tests so
> they can be run from the HDK. I don't recall it being declared 'done'?
>
> Regards,
> Tim
>
--
With best regards / с наилучшими пожеланиями,
Alexei Fedotov / Алексей Федотов,
http://www.telecom-express.ru/
http://harmony.apache.org/
http://www.expressaas.com/
http://openmeetings.googlecode.com/
[testing] pre-commit testing (was: Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java)
Posted by Tim Ellison <t....@gmail.com>.
On 17/Nov/2009 03:50, Nathan Beyer wrote:
> 2009/11/16 Alexei Fedotov <al...@gmail.com>:
>> Funny. We don't have a requirement that a committer have to test the
>> code before committing it here [1].
>
> I suppose some things are just assumed.
>
>> [1] http://wiki.apache.org/harmony/NewCommitter
Of course, we are all grown-ups. Mistakes happen.
How close are we to being able to run all the tests via Hudson?
I recall that there was discussion and work on packaging the tests so
they can be run from the HDK. I don't recall it being declared 'done'?
Regards,
Tim
Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java
Posted by Nathan Beyer <nb...@gmail.com>.
2009/11/16 Alexei Fedotov <al...@gmail.com>:
> Funny. We don't have a requirement that a committer have to test the
> code before committing it here [1].
I suppose some things are just assumed.
>
> [1] http://wiki.apache.org/harmony/NewCommitter
>
>
> On Fri, Nov 13, 2009 at 10:10 PM, Jesse Wilson <je...@google.com> wrote:
>> On Fri, Nov 13, 2009 at 9:41 AM, Tim Ellison <t....@gmail.com> wrote:
>>
>>> I took a look at the tests briefly, they are checking exception throwing
>>> compatibility, something that we aim to maintain with the RI as rightly
>>> or wrongly apps depend upon non-spec'd behavior.
>>>
>>
>> Sorry about this guys.
>>
>> One of the engineering problems I'm working through is that Android has its
>> own copy of the Harmony test suite. It has additional tests, plus some
>> changes to the original test suite. I'm not particularly happy having two
>> copies! Anyway, in submitting this change I'd run the BufferedReader changes
>> through our test suite, but not Harmony's, and as a consequence I've broken
>> some tests. I'll take better care going forward. The silver lining in this
>> is that Android has many tests to contribute in time.
>>
>> I've committed the fix as r835954.
>> http://svn.apache.org/viewvc?view=revision&revision=835954
>>
>
>
>
> --
> With best regards / с наилучшими пожеланиями,
> Alexei Fedotov / Алексей Федотов,
> http://www.telecom-express.ru/
> http://harmony.apache.org/
> http://www.expressaas.com/
> http://openmeetings.googlecode.com/
>
Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java
Posted by Alexei Fedotov <al...@gmail.com>.
Funny. We don't have a requirement that a committer have to test the
code before committing it here [1].
[1] http://wiki.apache.org/harmony/NewCommitter
On Fri, Nov 13, 2009 at 10:10 PM, Jesse Wilson <je...@google.com> wrote:
> On Fri, Nov 13, 2009 at 9:41 AM, Tim Ellison <t....@gmail.com> wrote:
>
>> I took a look at the tests briefly, they are checking exception throwing
>> compatibility, something that we aim to maintain with the RI as rightly
>> or wrongly apps depend upon non-spec'd behavior.
>>
>
> Sorry about this guys.
>
> One of the engineering problems I'm working through is that Android has its
> own copy of the Harmony test suite. It has additional tests, plus some
> changes to the original test suite. I'm not particularly happy having two
> copies! Anyway, in submitting this change I'd run the BufferedReader changes
> through our test suite, but not Harmony's, and as a consequence I've broken
> some tests. I'll take better care going forward. The silver lining in this
> is that Android has many tests to contribute in time.
>
> I've committed the fix as r835954.
> http://svn.apache.org/viewvc?view=revision&revision=835954
>
--
With best regards / с наилучшими пожеланиями,
Alexei Fedotov / Алексей Федотов,
http://www.telecom-express.ru/
http://harmony.apache.org/
http://www.expressaas.com/
http://openmeetings.googlecode.com/
Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java
Posted by Jesse Wilson <je...@google.com>.
On Fri, Nov 13, 2009 at 9:41 AM, Tim Ellison <t....@gmail.com> wrote:
> I took a look at the tests briefly, they are checking exception throwing
> compatibility, something that we aim to maintain with the RI as rightly
> or wrongly apps depend upon non-spec'd behavior.
>
Sorry about this guys.
One of the engineering problems I'm working through is that Android has its
own copy of the Harmony test suite. It has additional tests, plus some
changes to the original test suite. I'm not particularly happy having two
copies! Anyway, in submitting this change I'd run the BufferedReader changes
through our test suite, but not Harmony's, and as a consequence I've broken
some tests. I'll take better care going forward. The silver lining in this
is that Android has many tests to contribute in time.
I've committed the fix as r835954.
http://svn.apache.org/viewvc?view=revision&revision=835954
Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java
Posted by Tim Ellison <t....@gmail.com>.
On 13/Nov/2009 17:27, Jesse Wilson wrote:
> On Fri, Nov 13, 2009 at 9:22 AM, Tim Ellison <t....@gmail.com> wrote:
>
>> This commit breaks a couple of existing compatibility tests for
>> BufferedReader:
>>
>
> Yup, I saw this and I'm fixing it. I suspect it's just a bogus test. Sorry
> about that!
I took a look at the tests briefly, they are checking exception throwing
compatibility, something that we aim to maintain with the RI as rightly
or wrongly apps depend upon non-spec'd behavior.
Regards,
Tim
Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java
Posted by Jesse Wilson <je...@google.com>.
On Fri, Nov 13, 2009 at 9:22 AM, Tim Ellison <t....@gmail.com> wrote:
> This commit breaks a couple of existing compatibility tests for
> BufferedReader:
>
Yup, I saw this and I'm fixing it. I suspect it's just a bogus test. Sorry
about that!
Re: svn commit: r835212 - /harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java
Posted by Tim Ellison <t....@gmail.com>.
This commit breaks a couple of existing compatibility tests for
BufferedReader:
----------------------------------------
java.lang.NullPointerException: buffer is null
at java.io.BufferedReader.read(BufferedReader.java:282)
at
org.apache.harmony.luni.tests.java.io.BufferedReaderTest.test_read_$CII_Exception(BufferedReaderTest.java:382)
at java.lang.reflect.VMReflection.invokeMethod(VMReflection.java)
----------------------------------------
and
----------------------------------------
junit.framework.AssertionFailedError: should throw IOException
at
org.apache.harmony.luni.tests.java.io.BufferedReaderTest.test_reset_IOException(BufferedReaderTest.java:538)
at java.lang.reflect.VMReflection.invokeMethod(VMReflection.java)
----------------------------------------
Regards,
Tim
On 12/Nov/2009 03:25, jessewilson@apache.org wrote:
> Author: jessewilson
> Date: Thu Nov 12 03:25:52 2009
> New Revision: 835212
>
> URL: http://svn.apache.org/viewvc?rev=835212&view=rev
> Log:
> Cleanup and bugfix BufferedReader.
>
> This change includes the following functional changes:
> - changing read to not clear the mark upon reading EOF. The previous behaviour was incorrect.
> - changing read(char[], int, int) to use the 'read directly from the source stream' shortcut when the mark has exceeded its limit. Previously we took the shortcut only when the mark was unset.
>
> And these nonfunctional changes:
> - rewrote read(char[], int, int). The new revision contains only one call to 'System.arrayCopy' and the related bookkeeping. Previously there was one call before the loop, and another call in the loop.
> - renamed markpos to mark
> - renamed marklimit to markLimit
> - renamed count to end (it isn't a count, it's a position)
> - renamed fillbuf() to fillBuf()
> - simplified conditions that used >= when > was impossible
> - reducing the number of field reads where convenient
>
> This tidy up is intended to prepare BufferedReader for some further bugfixing. We've seen some bugs reported against readLine() and I found it quite frustrating to work on the code when the names were wrong (ie. count) or of a foreign style (such as fillbuf()). I also attempted to document what the heck was going on in the more sophiticated methods.
>
> Modified:
> harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/io/BufferedReader.java