Hi, Jim
I have some comments on the CSR and the Webrev.
CSR:
====
"This method takes the receiver String a replaces escape sequences with
character equivalents."
a -> and
--
In the specification, I like emphasizing that nothing happens to 'this',
but rather to the returned string, so maybe something like:
"Returns a string with all escape sequences translated into characters
represented by..."
Webrev:
=======
src/java.base/share/classes/java/lang/String.java
3006 case '\'': case '\"': case '\\':
I would find this easier to read with one case per line, so:
case '\'':
case '\"':
case '\\':
break;
test/jdk/java/lang/String/TranslateEscapes.java
The test should cover octal escapes, and cases that throw
IllegalArgumentException.
Why does the test need to run in othervm ?
Thanks,
-Brent
On 5/21/19 12:24 PM, Jim Laskey wrote:
Revised at
http://cr.openjdk.java.net/~jlaskey/8223780/webrev-03/src/java.base/share/classes/java/lang/String.java.frames.html
<http://cr.openjdk.java.net/~jlaskey/8223780/webrev-03/src/java.base/share/classes/java/lang/String.java.frames.html>
On May 21, 2019, at 3:23 PM, Jim Laskey <[email protected]> wrote:
On May 21, 2019, at 2:57 PM, Ivan Gerasimov <[email protected]> wrote:
Hi Jim!
A few comments:
1)
Probably, there's no need to update ch in these cases:
case '\'':
ch = '\'';
break;
case '\"':
ch = '\"';
break;
True
2)
Character.digit(ch, 8) will accept non-Latin1 digits.
So, a sequence \\3\uFF17\uFF17 will be parsed as \\377.
(Note, that the first digit still can only be from range '0'-'9').
Also true. Subtlety in UnicodeReader.
3)
It's not obvious how this condition can be triggered:
if (0377 < code) {
throw new MalformedEscapeException(from);
}
I might be missing something, but I cannot see how 'code' can become > 0377.
I removed this check in round 2.
4)
throw new MalformedEscapeException(from);
This will report the next index after the error. Was it intentional?
I switched to using an IllegalArgumentError and displaying the actual character.
With kind regards,
Ivan
Much thanks.
On 5/21/19 7:56 AM, Jim Laskey wrote:
Please do a code review of the new String:: translateEscapes instance method.
This instance method is being introduced to support JEP-355: Text Blocks, by
translating escape sequences in the text block content.
Thank you.
-- Jim
webrev: http://cr.openjdk.java.net/~jlaskey/8223780/webrev-01
<http://cr.openjdk.java.net/~jlaskey/8223780/webrev-01>
jbs: https://bugs.openjdk.java.net/browse/JDK-8223780
<https://bugs.openjdk.java.net/browse/JDK-8223780>
csr: https://bugs.openjdk.java.net/browse/JDK-8223781
<https://bugs.openjdk.java.net/browse/JDK-8223781>
jep: https://bugs.openjdk.java.net/browse/JDK-8222530
<https://bugs.openjdk.java.net/browse/JDK-8222530>
--
With kind regards,
Ivan Gerasimov
