[15] 8235792: LineNumberReader.getLineNumber() behavior is inconsistent with respect to EOF
This issue [1] attempts to reinstate the change for [2] which was backed out to fix [3] thereby maintaining longstanding behavior. The proposed change [4] modifies the specification to increment the line number also when EOF is encountered and is not immediately preceded by a line terminator. If this all seems reasonable then a CSR will be filed after the code review. One thing I am uncertain about is the change to skip() at lines 293-295 of LineNumberReader. I am not sure that those lines should be added. Thanks, Brian [1] https://bugs.openjdk.java.net/browse/JDK-8235792 [2] https://bugs.openjdk.java.net/browse/JDK-8230342 [3] https://bugs.openjdk.java.net/browse/JDK-8235668 [4] http://cr.openjdk.java.net/~bpb/8235792/webrev.00/
Ping ...
On Jan 16, 2020, at 5:35 PM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
This issue [1] attempts to reinstate the change for [2] which was backed out to fix [3] thereby maintaining longstanding behavior. The proposed change [4] modifies the specification to increment the line number also when EOF is encountered and is not immediately preceded by a line terminator. If this all seems reasonable then a CSR will be filed after the code review.
One thing I am uncertain about is the change to skip() at lines 293-295 of LineNumberReader. I am not sure that those lines should be added.
Thanks,
Brian
[1] https://bugs.openjdk.java.net/browse/JDK-8235792 [2] https://bugs.openjdk.java.net/browse/JDK-8230342 [3] https://bugs.openjdk.java.net/browse/JDK-8235668 [4] http://cr.openjdk.java.net/~bpb/8235792/webrev.00/
Hi Brian, The spec text describing the current behavior is ok. On 1/16/20 8:35 PM, Brian Burkhalter wrote:
This issue [1] attempts to reinstate the change for [2] which was backed out to fix [3] thereby maintaining longstanding behavior. The proposed change [4] modifies the specification to increment the line number also when EOF is encountered and is not immediately preceded by a line terminator. If this all seems reasonable then a CSR will be filed after the code review.
LineNumberReader: 55: I would probably use 'EOL' instead of 'TERM'.
One thing I am uncertain about is the change to skip() at lines 293-295 of LineNumberReader. I am not sure that those lines should be added.
Leave them in. Not changing prevChar would be misleading since the prevChar isn't adjecent to the next read. Roger
Thanks,
Brian
[1] https://bugs.openjdk.java.net/browse/JDK-8235792 [2] https://bugs.openjdk.java.net/browse/JDK-8230342 [3] https://bugs.openjdk.java.net/browse/JDK-8235668 [4] http://cr.openjdk.java.net/~bpb/8235792/webrev.00/
Hi Roger, Thanks for the review.
On Jan 31, 2020, at 12:05 PM, Roger Riggs <Roger.Riggs@oracle.com> wrote:
The spec text describing the current behavior is ok.
On 1/16/20 8:35 PM, Brian Burkhalter wrote:
This issue [1] attempts to reinstate the change for [2] which was backed out to fix [3] thereby maintaining longstanding behavior. The proposed change [4] modifies the specification to increment the line number also when EOF is encountered and is not immediately preceded by a line terminator. If this all seems reasonable then a CSR will be filed after the code review.
LineNumberReader:
55: I would probably use 'EOL' instead of 'TERM’.
Yes, that is better.
One thing I am uncertain about is the change to skip() at lines 293-295 of LineNumberReader. I am not sure that those lines should be added.
Leave them in. Not changing prevChar would be misleading since the prevChar isn't adjecent to the next read.
OK. I’ll change /TERM/EOL/ but not post another webrev unless there are further comments. I will wait to check it in until Monday in case someone has comments. Thanks, Brian
On 31/01/2020 20:10, Brian Burkhalter wrote:
: OK. I’ll change /TERM/EOL/ but not post another webrev unless there are further comments. I will wait to check it in until Monday in case someone has comments.
I think it would be good to more eyes on this as LNR has a history of resisting change. I think you'll need a CSR too this re-specifies when the conditions when the line number is incremented. -Alan.
On Jan 31, 2020, at 12:20 PM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 31/01/2020 20:10, Brian Burkhalter wrote:
: OK. I’ll change /TERM/EOL/ but not post another webrev unless there are further comments. I will wait to check it in until Monday in case someone has comments.
I think it would be good to more eyes on this as LNR has a history of resisting change.
I’ll hold off on it then for a while.
I think you'll need a CSR too this re-specifies when the conditions when the line number is incremented.
Yes, I agree. Thanks, Brian
Might anyone else have any comments on this thread the original post in which was [1]. Thanks, Brian [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-January/064417.htm...
On Jan 31, 2020, at 12:21 PM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
On Jan 31, 2020, at 12:20 PM, Alan Bateman <Alan.Bateman@oracle.com <mailto:Alan.Bateman@oracle.com>> wrote:
On 31/01/2020 20:10, Brian Burkhalter wrote:
: OK. I’ll change /TERM/EOL/ but not post another webrev unless there are further comments. I will wait to check it in until Monday in case someone has comments.
I think it would be good to more eyes on this as LNR has a history of resisting change.
I’ll hold off on it then for a while.
I think you'll need a CSR too this re-specifies when the conditions when the line number is incremented.
Yes, I agree.
:) Still looks fine. On 3/5/20 4:53 PM, Brian Burkhalter wrote:
Might anyone else have any comments on this thread the original post in which was [1].
Thanks,
Brian
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-January/064417.htm...
On Jan 31, 2020, at 12:21 PM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
On Jan 31, 2020, at 12:20 PM, Alan Bateman <Alan.Bateman@oracle.com <mailto:Alan.Bateman@oracle.com>> wrote:
On 31/01/2020 20:10, Brian Burkhalter wrote:
: OK. I’ll change /TERM/EOL/ but not post another webrev unless there are further comments. I will wait to check it in until Monday in case someone has comments.
I think it would be good to more eyes on this as LNR has a history of resisting change. I’ll hold off on it then for a while.
I think you'll need a CSR too this re-specifies when the conditions when the line number is incremented. Yes, I agree.
Thanks, Roger. I’ll let it hang until next week and then file a CSR. Brian
On Mar 6, 2020, at 10:49 AM, Roger Riggs <Roger.Riggs@oracle.com> wrote:
:) Still looks fine.
On 3/5/20 4:53 PM, Brian Burkhalter wrote:
Might anyone else have any comments on this thread the original post in which was [1].
Thanks,
Brian
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-January/064417.htm... <http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-January/064417.html>
An updated webrev is at [1] and a CSR has been filed [2]. Thanks, Brian [1] http://cr.openjdk.java.net/~bpb/8235792/webrev.01/ [2] https://bugs.openjdk.java.net/browse/JDK-8241020
On Mar 6, 2020, at 11:13 AM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
Thanks, Roger. I’ll let it hang until next week and then file a CSR.
Brian
On Mar 6, 2020, at 10:49 AM, Roger Riggs <Roger.Riggs@oracle.com> wrote:
:) Still looks fine.
On 3/5/20 4:53 PM, Brian Burkhalter wrote:
Might anyone else have any comments on this thread the original post in which was [1].
Thanks,
Brian
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-January/064417.htm... <http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-January/064417.html>
Another webrev [1] which is adjusted from the previous one per the comments on the CSR [2] is available for review. The only change is to the class-level specification: --- a/src/java.base/share/classes/java/io/LineNumberReader.java +++ b/src/java.base/share/classes/java/io/LineNumberReader.java @@ -41,7 +41,8 @@ * * <p> A line is considered to be <a id="lt">terminated</a> by any one of a * line feed ('\n'), a carriage return ('\r'), or a carriage return followed - * immediately by a linefeed. + * immediately by a linefeed, or any of the previous terminators followed by + * end of stream, or end of stream not preceded by another terminator. * Thanks, Brian
On Mar 13, 2020, at 10:28 AM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
An updated webrev is at [1] and a CSR has been filed [2].
[1] http://cr.openjdk.java.net/~bpb/8235792/webrev.02/ [2] https://bugs.openjdk.java.net/browse/JDK-8241020
The CSR [2] has been approved so unless there are further comments I’ll push this change [1] this week. Thanks, Brian
On Mar 19, 2020, at 7:43 AM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
Another webrev [1] which is adjusted from the previous one per the comments on the CSR [2] is available for review. The only change is to the class-level specification:
--- a/src/java.base/share/classes/java/io/LineNumberReader.java +++ b/src/java.base/share/classes/java/io/LineNumberReader.java @@ -41,7 +41,8 @@ * * <p> A line is considered to be <a id="lt">terminated</a> by any one of a * line feed ('\n'), a carriage return ('\r'), or a carriage return followed - * immediately by a linefeed. + * immediately by a linefeed, or any of the previous terminators followed by + * end of stream, or end of stream not preceded by another terminator. *
Thanks,
Brian
On Mar 13, 2020, at 10:28 AM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
An updated webrev is at [1] and a CSR has been filed [2].
[1] http://cr.openjdk.java.net/~bpb/8235792/webrev.02/ [2] https://bugs.openjdk.java.net/browse/JDK-8241020
Push to JDK 16 that is.
On Jul 20, 2020, at 10:04 AM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
The CSR [2] has been approved so unless there are further comments I’ll push this change [1] this week.
Thanks,
Brian
On Mar 19, 2020, at 7:43 AM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
Another webrev [1] which is adjusted from the previous one per the comments on the CSR [2] is available for review. The only change is to the class-level specification:
--- a/src/java.base/share/classes/java/io/LineNumberReader.java +++ b/src/java.base/share/classes/java/io/LineNumberReader.java @@ -41,7 +41,8 @@ * * <p> A line is considered to be <a id="lt">terminated</a> by any one of a * line feed ('\n'), a carriage return ('\r'), or a carriage return followed - * immediately by a linefeed. + * immediately by a linefeed, or any of the previous terminators followed by + * end of stream, or end of stream not preceded by another terminator. *
Thanks,
Brian
On Mar 13, 2020, at 10:28 AM, Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
An updated webrev is at [1] and a CSR has been filed [2].
[1] http://cr.openjdk.java.net/~bpb/8235792/webrev.02/ [2] https://bugs.openjdk.java.net/browse/JDK-8241020
participants (3)
-
Alan Bateman
-
Brian Burkhalter
-
Roger Riggs