RFR - JDK-8223780 String::translateEscapes (Preview)

[Jim Laskey]

Thank you. Will integrate your suggested changes.

> On May 21, 2019, at 8:15 PM, Brent Christian <brent.christian at oracle.com> wrote:
> 
> 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 <james.laskey at oracle.com> wrote:
>>> 
>>> 
>>> 
>>>> On May 21, 2019, at 2:57 PM, Ivan Gerasimov <ivan.gerasimov at oracle.com> 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
>>>> 
>>>