RFR: [15] JDK-8236030: Cleanup use of String.toCharArray

[Hannes Wallnöfer]

Yes, I intended it in the way Claes explained it, and I also assumed this was a bug.

In the case of removeTrailingWhitespace even something like "x  " would return unchanged, because the for-loop stops before reaching the first character. I wonder if this case of a single non-whitespace text never occurs, or if just nobody ever noticed it?

Hannes


> Am 18.12.2019 um 16:18 schrieb Jonathan Gibbons <jonathan.gibbons at oracle.com>:
> 
> Claes,
> 
> Thanks for the clarification.   It sounds like the old behavior was a bug.
> 
> I note that I did do before/after comparison for the main docs build
> and for all the many runs of javadoc performed by the jtreg tests,
> and all compared OK.
> 
> -- Jon
> 
> On 12/18/19 7:11 AM, Claes Redestad wrote:
>> Sorry for jumping in: I initially misread Hannes' observation in the
>> same way, but I think he meant that "   ".strip() will return "", while
>> removeTrailingWhitespace("    ") will return "    ", which is a more
>> subtle difference in behavior.
>> 
>> /Claes
>> 
>> On 2019-12-18 15:46, Jonathan Gibbons wrote:
>>> Hannes,
>>> 
>>> Thanks for the review.
>>> 
>>> My reading of the code for String.strip.... is that the original string is returned if there is no whitespace to be removed ... in other words, the new methods should have the same behavior as the removed methods.
>>> 
>>> -- Jon
>>> 
>>> On 12/18/19 4:56 AM, Hannes Wallnöfer wrote:
>>>> Looks good.
>>>> 
>>>> I noticed that the removed methods would return the original string if they didn’t encounter a non-whitespace character, but I guess that wasn’t actually part of their intended behaviour.
>>>> 
>>>> Hannes
>>>> 
>>>>> Am 16.12.2019 um 23:47 schrieb Jonathan Gibbons <jonathan.gibbons at oracle.com>:
>>>>> 
>>>>> Updated webrev based on Ivan's suggestion to use one of stripLeading(), stripTrailing(), or strip() instead of the old methods, which are now removed
>>>>> 
>>>>> -- Jon
>>>>> 
>>>>> http://cr.openjdk.java.net/~jjg/8236030/webrev.01/
>>>>> 
>>>>> 
>>>>> 
>>>>> On 12/16/2019 01:33 PM, Jonathan Gibbons wrote:
>>>>>> Ivan,
>>>>>> 
>>>>>> Great suggestion, and sets up the possibility to  use strip() when both leading and trailing whitespace should be removed.
>>>>>> 
>>>>>> The only slight change in semantics would be that these methods work in terms of code-points, not characters, and javadoc has not (yet?) been adapted to use code-points throughout.
>>>>>> 
>>>>>> -- Jon
>>>>>> 
>>>>>> 
>>>>>> On 12/16/2019 01:23 PM, Ivan Gerasimov wrote:
>>>>>>> Hello!
>>>>>>> 
>>>>>>> Can String.stripLeading()/stripTrailing() methods be used instead, or is there a reason to avoid them?
>>>>>>> 
>>>>>>> With kind regards,
>>>>>>> 
>>>>>>> Ivan
>>>>>>> 
>>>>>>> 
>>>>>>> On 12/16/19 1:10 PM, Jonathan Gibbons wrote:
>>>>>>>> Please review a tiny change to eliminate a string copy into a temporary character buffer when removing leading or trailing whitespace from a string.
>>>>>>>> 
>>>>>>>> The affected methods are called within the main code to translate doc comments to HTML.  This is noreg-cleanup/no-reg-trivial, so no new additional tests.
>>>>>>>> 
>>>>>>>> Webrev below, but the patch is also included here:
>>>>>>>> 
>>>>>>>> --- a/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java Mon Dec 16 15:33:03 2019 -0500
>>>>>>>> +++ b/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java Mon Dec 16 13:07:18 2019 -0800
>>>>>>>> @@ -1604,22 +1604,19 @@
>>>>>>>>       }
>>>>>>>> 
>>>>>>>>       private String removeTrailingWhitespace(String text) {
>>>>>>>> -        char[] buf = text.toCharArray();
>>>>>>>> -        for (int i = buf.length - 1; i > 0 ; i--) {
>>>>>>>> -            if (!Character.isWhitespace(buf[i]))
>>>>>>>> -                return text.substring(0, i + 1);
>>>>>>>> +        int i = text.length() - 1;
>>>>>>>> +        while (i >= 0 && Character.isWhitespace(text.charAt(i))) {
>>>>>>>> +            i--;
>>>>>>>>           }
>>>>>>>> -        return text;
>>>>>>>> +        return i == text.length() - 1 ? text : text.substring(0, i + 1);
>>>>>>>>       }
>>>>>>>> 
>>>>>>>>       private String removeLeadingWhitespace(String text) {
>>>>>>>> -        char[] buf = text.toCharArray();
>>>>>>>> -        for (int i = 0; i < buf.length; i++) {
>>>>>>>> -            if (!Character.isWhitespace(buf[i])) {
>>>>>>>> -                return text.substring(i);
>>>>>>>> -            }
>>>>>>>> +        int i = 0;
>>>>>>>> +        while (i < text.length() && Character.isWhitespace(text.charAt(i))) {
>>>>>>>> +            i++;
>>>>>>>>           }
>>>>>>>> -        return text;
>>>>>>>> +        return i == 0 ? text : text.substring(i);
>>>>>>>>       }
>>>>>>>> 
>>>>>>>>       /**
>>>>>>>> 
>>>>>>>> 
>>>>>>>> -- Jon
>>>>>>>> 
>>>>>>>> 
>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8236030
>>>>>>>> Webrev: http://cr.openjdk.java.net/~jjg/8236030/webrev/
>>>>>>>>