add replace_char() method to SkString API (skbug.com/2651)

add replace_char() method to SkString API (skbug.com/2651)

Provide a method in the SkString API to replace all occurrences of one
char with another.

BUG=skia:2651

[rs.prinja, 2014-06-11 05:14:55]

The CQ bit was checked by rs.prinja@samsung.com

[rs.prinja, 2014-06-11 05:15:01]

The CQ bit was unchecked by rs.prinja@samsung.com

[epoger, 2014-06-11 11:01:33]

I will defer to reed on this core API change (you'll need his approval to commit
into these directories anyway), but I've made some suggestions anyway.

https://codereview.chromium.org/322253005/diff/1/include/core/SkString.h
File include/core/SkString.h (right):

https://codereview.chromium.org/322253005/diff/1/include/core/SkString.h#newc...
include/core/SkString.h:193: 
Please add unittests in
https://skia.googlesource.com/skia/+/master/tests/StringTest.cpp

https://codereview.chromium.org/322253005/diff/1/include/core/SkString.h#newc...
include/core/SkString.h:195: void replace_char(const char oldChar, const char
newChar);
For consistency, please call this either replace() or replaceChar().  I'm not
sure which is better.

https://codereview.chromium.org/322253005/diff/1/src/core/SkString.cpp
File src/core/SkString.cpp (right):

https://codereview.chromium.org/322253005/diff/1/src/core/SkString.cpp#newcod...
src/core/SkString.cpp:563: if (oldChar == this->operator[](i)) {
Instead of going through the [] operator, I suspect it would be more efficient
to do something like this:

size_t len = this->size();
if (len > 0) {
  char* writable = this->writable_str();
  for (size_t i = len - 1; i >= 0; --i) {
    if (oldChar == writable[i]) {
      writable[i] = newChar;
    }
  }
}

[rs.prinja, 2014-06-11 12:08:03]

https://codereview.chromium.org/322253005/diff/1/include/core/SkString.h
File include/core/SkString.h (right):

https://codereview.chromium.org/322253005/diff/1/include/core/SkString.h#newc...
include/core/SkString.h:193: 
On 2014/06/11 11:01:33, epoger wrote:
> Please add unittests in
> https://skia.googlesource.com/skia/+/master/tests/StringTest.cpp

Done.

https://codereview.chromium.org/322253005/diff/1/include/core/SkString.h#newc...
include/core/SkString.h:195: void replace_char(const char oldChar, const char
newChar);
On 2014/06/11 11:01:33, epoger wrote:
> For consistency, please call this either replace() or replaceChar().  I'm not
> sure which is better.

I'm calling it replaceChar for the time being. Will reupload if replace() or
some other name sounds better to everyone.

https://codereview.chromium.org/322253005/diff/1/src/core/SkString.cpp
File src/core/SkString.cpp (right):

https://codereview.chromium.org/322253005/diff/1/src/core/SkString.cpp#newcod...
src/core/SkString.cpp:563: if (oldChar == this->operator[](i)) {
On 2014/06/11 11:01:33, epoger wrote:
> Instead of going through the [] operator, I suspect it would be more efficient
> to do something like this:
> 
> size_t len = this->size();
> if (len > 0) {
>   char* writable = this->writable_str();
>   for (size_t i = len - 1; i >= 0; --i) {
>     if (oldChar == writable[i]) {
>       writable[i] = newChar;
>     }
>   }
> }

Done, although since unsigned always compares as >= 0 I ran the loop from 0 to
len-1 instead.

[reed1, 2014-06-11 12:11:35]

esp. since the caller is in a tool, and not in the core, I suggest we just make
this a local helper function in the test. Its impl seems to be done with just
public api calls.

[epoger, 2014-06-11 13:27:23]

On 2014/06/11 12:11:35, reed1 wrote:
> esp. since the caller is in a tool, and not in the core, I suggest we just
make
> this a local helper function in the test. Its impl seems to be done with just
> public api calls.

Mike- by "this" I assume you mean the new replaceChar() method?

Where exactly (header filename) do you propose it go, where it can be called by
various tools, etc?  Maybe a new SkStringUtils.h within
https://skia.googlesource.com/skia/+/master/include/utils/ ?

[reed1, 2014-06-11 13:32:52]

at the moment is is just called by one test, so I would place it in that .cpp.

For broader availability, we've started adding helpers in sk_tool_utils.h

[epoger, 2014-06-11 13:36:26]

On 2014/06/11 13:32:52, reed1 wrote:
> at the moment is is just called by one test, so I would place it in that .cpp.

Within this CL, yes, but the point (according to
http://code.google.com/p/skia/issues/detail?id=2651 ) is for it to be called
from various files within https://skia.googlesource.com/skia/+/master/tools/

> 
> For broader availability, we've started adding helpers in sk_tool_utils.h

Sounds like https://skia.googlesource.com/skia/+/master/tools/sk_tool_utils.h is
the place for this function.  I don't know where the unittest goes, in that
case.

[reed1, 2014-06-11 13:40:50]

ah, my bad. I should have read the accompanying bug. Now I see the motivation.

https://codereview.chromium.org/322253005/diff/20001/include/core/SkString.h
File include/core/SkString.h (right):

https://codereview.chromium.org/322253005/diff/20001/include/core/SkString.h#...
include/core/SkString.h:194: /* Replace all occurrences of oldChar with newChar
*/
Please document what happens
- if oldChar or newChar is 0
- if we allow oldChar or newChar to be 8-bit (i.e. part of a compound utf8
sequence

https://codereview.chromium.org/322253005/diff/20001/include/core/SkString.h#...
include/core/SkString.h:195: void replaceChar(const char oldChar, const char
newChar);
we don't say 'const' for non-ptr/ref parameters

[rs.prinja, 2014-06-13 06:39:31]

On 2014/06/11 13:40:50, reed1 wrote:
> ah, my bad. I should have read the accompanying bug. Now I see the motivation.
> 
> https://codereview.chromium.org/322253005/diff/20001/include/core/SkString.h
> File include/core/SkString.h (right):
> 
>
https://codereview.chromium.org/322253005/diff/20001/include/core/SkString.h#...
> include/core/SkString.h:194: /* Replace all occurrences of oldChar with
newChar
> */
> Please document what happens
> - if oldChar or newChar is 0
> - if we allow oldChar or newChar to be 8-bit (i.e. part of a compound utf8
> sequence
> 
>
https://codereview.chromium.org/322253005/diff/20001/include/core/SkString.h#...
> include/core/SkString.h:195: void replaceChar(const char oldChar, const char
> newChar);
> we don't say 'const' for non-ptr/ref parameters

With the current patchset, if

1. newchar == 0, then, fRec->fLength does not change, yet when printed, the
string terminates at the new \0.
2. oldChar == 0, then, no replacement occurs, since we never hit the \0 in the
scan.

Also, if

3. newChar is 8-bit, the string prints with U+FFFD (i.e. �) everywhere the
replacement should have happened.
4. oldChar is 8-bit, no replacement occurs.

Are all of these desired behaviour? I'm laying off on documenting for now,
please let me know what desired behaviour should be. Then I'll reupload with
documentation, and possibly new functions if need be (replaceUnichar() ???)

[tfarina, 2014-06-13 17:02:10]

https://codereview.chromium.org/322253005/diff/20001/tests/StringTest.cpp
File tests/StringTest.cpp (right):

https://codereview.chromium.org/322253005/diff/20001/tests/StringTest.cpp#new...
tests/StringTest.cpp:151: a.set("hello world");
If Elliot and Mike don't mind, could you set up a separate test for this? e.g.:

DEF_TEST(String_ReplaceChar, r) {
  ...
}

[rs.prinja, 2014-06-17 04:18:30]

On 2014/06/13 17:02:10, tfarina wrote:
> https://codereview.chromium.org/322253005/diff/20001/tests/StringTest.cpp
> File tests/StringTest.cpp (right):
> 
>
https://codereview.chromium.org/322253005/diff/20001/tests/StringTest.cpp#new...
> tests/StringTest.cpp:151: a.set("hello world");
> If Elliot and Mike don't mind, could you set up a separate test for this?
e.g.:
> 
> DEF_TEST(String_ReplaceChar, r) {
>   ...
> }

Done, and also is there a need for a substring replacement function? As in

    void replaceStr(const char oldStr[], const char newStr[]);

?

[epoger, 2014-06-17 10:26:14]

On 2014/06/17 04:18:30, rs.prinja wrote:
> On 2014/06/13 17:02:10, tfarina wrote:
> > https://codereview.chromium.org/322253005/diff/20001/tests/StringTest.cpp
> > File tests/StringTest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/322253005/diff/20001/tests/StringTest.cpp#new...
> > tests/StringTest.cpp:151: a.set("hello world");
> > If Elliot and Mike don't mind, could you set up a separate test for this?
> e.g.:
> > 
> > DEF_TEST(String_ReplaceChar, r) {
> >   ...
> > }
> 
> Done, and also is there a need for a substring replacement function? As in
> 
>     void replaceStr(const char oldStr[], const char newStr[]);
> 
> ?

[Traveling until July 7, so deferring to Mike for all further reviews]

[scroggo, 2014-06-17 13:49:21]

On 2014/06/17 04:18:30, rs.prinja wrote:
> On 2014/06/13 17:02:10, tfarina wrote:
> > https://codereview.chromium.org/322253005/diff/20001/tests/StringTest.cpp
> > File tests/StringTest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/322253005/diff/20001/tests/StringTest.cpp#new...
> > tests/StringTest.cpp:151: a.set("hello world");
> > If Elliot and Mike don't mind, could you set up a separate test for this?
> e.g.:
> > 
> > DEF_TEST(String_ReplaceChar, r) {
> >   ...
> > }
> 
> Done, and also is there a need for a substring replacement function? As in
> 
>     void replaceStr(const char oldStr[], const char newStr[]);
> 
> ?

Not that I'm aware of; I'd say let's wait until we think we need it.

Is the plan to remove the existing functions in a different CL?

[mtklein, 2014-06-17 14:31:18]

I'd rather not add methods that can be implemented just as efficiently with a
public API, particularly here in a class exposed publicly.

Any chance you could implement this in and use it from SkStringUtils{.h,.cpp} if
they move to src/core?  (Hopefully on its way here:
https://codereview.chromium.org/335413004/)

[rs.prinja, 2014-06-18 01:11:32]

On 2014/06/17 14:31:18, mtklein wrote:
> I'd rather not add methods that can be implemented just as efficiently with a
> public API, particularly here in a class exposed publicly.
> 
> Any chance you could implement this in and use it from SkStringUtils{.h,.cpp}
if
> they move to src/core?  (Hopefully on its way here:
> https://codereview.chromium.org/335413004/)

So to clarify, replaceChar() remains in SkString{.h,.cpp} and replaceStr() goes
in SkStringUtils? Or do I also shift replaceChar() to SkStringUtils?

[rs.prinja, 2014-06-24 08:36:47]

ping, PTAL and let me know how to proceed.

thank you

[tfarina, 2014-07-04 00:54:53]

Reducing the reviewers list here.

[mtklein, 2014-07-07 13:06:02]

> So to clarify, replaceChar() remains in SkString{.h,.cpp} and replaceStr()
goes
> in SkStringUtils? Or do I also shift replaceChar() to SkStringUtils?

Sorry!  Seems I haven't been CC'd on these messages.  I'd prefer both
replaceChar() and any replaceStr() method you write to go in SkStringUtils.

[tfarina, 2014-08-13 17:23:09]

rs.prinja do you mind if I take this from you?

[reed1, 2014-08-13 19:13:36]

https://codereview.chromium.org/322253005/diff/40001/include/core/SkString.h
File include/core/SkString.h (right):

https://codereview.chromium.org/322253005/diff/40001/include/core/SkString.h#...
include/core/SkString.h:194: void replaceChar(char oldChar, char newChar);
ick -- are we logically trying to replace a byte, or a "character". If the
latter, then pass SkUnichar. If the former... really?

[dw.im, 2014-09-15 00:34:12]

dw.im@samsung.com changed reviewers:
+ dw.im@samsung.com

[dw.im, 2014-09-15 00:34:12]

Dear Rohan, will you keep working on this?

[rohan.prinja, 2014-09-15 20:09:10]

@tfarina, mtklein, dw.im

Terribly sorry for not responding earlier.

@tfarina

No problem

[tfarina, 2014-10-11 21:09:56]

I have made an initial CL -> https://codereview.chromium.org/646213002.

More to come after it lands.