Thank you. Will integrate your suggested changes.
> On May 21, 2019, at 8:15 PM, Brent Christian <[email protected]>
> 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 <[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
>>>>
>>>
