Bug 239500 – Replace String::replaceWithLiteral() with a String::replace() overload that takes in an ASCIILiteral

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Chris Dumez]

Replace String::replaceWithLiteral() with a String::replace() overload that takes in an ASCIILiteral.

[Chris Dumez]

Created attachment 457897 [details]
Patch

[Darin Adler]

Comment on attachment 457897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457897&action=review

> Source/WTF/wtf/text/WTFString.h:198
> +    String& replace(UChar target, ASCIILiteral);

I would sort this up with the UChar/StringView overload above it.

> Source/WebCore/editing/markup.cpp:423
> +    return result.toString().replace('\0', ""_s);

I wish it was clearer when to use ""_s and when to use emptyString(). I suppose that ideally the two would always generate identical code, the more optimal of the two.

Another possibility would be to add a function named something like removeAll. Makes me realize that replace should maybe be named replaceAll.

> Tools/TestWebKitAPI/Tests/WTF/WTFString.cpp:247
> +    testString.replace('2', "3"_s);

If there were a lot of callers like this we’d probably want a replace(UChar, UChar).

[Chris Dumez]

Created attachment 457920 [details]
Patch

[Sam Weinig]

(In reply to Darin Adler from comment #2)
> Comment on attachment 457897 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457897&action=review
> 
> 
> Another possibility would be to add a function named something like
> removeAll. Makes me realize that replace should maybe be named replaceAll.

Agree with both these.

> > Tools/TestWebKitAPI/Tests/WTF/WTFString.cpp:247
> > +    testString.replace('2', "3"_s);
> 
> If there were a lot of callers like this we’d probably want a replace(UChar,
> UChar).

There is one!

[Sam Weinig]

Should we = delete the overload that takes a const char* to ensure people aren't passing in a literal the other way?

[Chris Dumez]

(In reply to Sam Weinig from comment #5)
> Should we = delete the overload that takes a const char* to ensure people
> aren't passing in a literal the other way?

Sounds like a good idea. I'll look into that. We wouldn't want people to use the StringView overload when they have a string literal.

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> (In reply to Sam Weinig from comment #5)
> > Should we = delete the overload that takes a const char* to ensure people
> > aren't passing in a literal the other way?
> 
> Sounds like a good idea. I'll look into that. We wouldn't want people to use
> the StringView overload when they have a string literal.

It even found bad call sites lol :)

[Chris Dumez]

(In reply to Sam Weinig from comment #4)
> (In reply to Darin Adler from comment #2)
> > Comment on attachment 457897 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=457897&action=review
> > 
> > 
> > Another possibility would be to add a function named something like
> > removeAll. Makes me realize that replace should maybe be named replaceAll.
> 
> Agree with both these.

Same. Based on Darin's earlier feedback that it'd be nice if String didn't have functions that modify the String itself though, maybe free functions like:
```
makeStringByReplacingAll(const String&, UChar, ASCIILiteral/StringView/UChar)
makeStringByRemovingAll(const String&, UChar)
```

It'd be consistent with the makeStringByInserting() that I recently added.

[Chris Dumez]

Created attachment 457931 [details]
Patch

[Chris Dumez]

Created attachment 457934 [details]
Patch

[Chris Dumez]

Created attachment 457943 [details]
Patch

[EWS]

Committed r293056 (249789@main): <https://commits.webkit.org/249789@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457943 [details].

[Radar WebKit Bug Importer]

<rdar://problem/92001180>

[Sam Weinig]

(In reply to Chris Dumez from comment #8)
> (In reply to Sam Weinig from comment #4)
> > (In reply to Darin Adler from comment #2)
> > > Comment on attachment 457897 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=457897&action=review
> > > 
> > > 
> > > Another possibility would be to add a function named something like
> > > removeAll. Makes me realize that replace should maybe be named replaceAll.
> > 
> > Agree with both these.
> 
> Same. Based on Darin's earlier feedback that it'd be nice if String didn't
> have functions that modify the String itself though, maybe free functions
> like:
> ```
> makeStringByReplacingAll(const String&, UChar, ASCIILiteral/StringView/UChar)
> makeStringByRemovingAll(const String&, UChar)
> ```
> 
> It'd be consistent with the makeStringByInserting() that I recently added.

Another option would be making the member functions const and have them return a new WARN_UNUSED_RETURN String. If we did that, we could consider names like:

WARN_UNUSED_RETURN String stringByReplacingAll(UChar, ASCIILiteral/StringView/UChar) const
WARN_UNUSED_RETURN String stringByRemovingAll(UChar) const

[Chris Dumez]

(In reply to Sam Weinig from comment #14)
> (In reply to Chris Dumez from comment #8)
> > (In reply to Sam Weinig from comment #4)
> > > (In reply to Darin Adler from comment #2)
> > > > Comment on attachment 457897 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=457897&action=review
> > > > 
> > > > 
> > > > Another possibility would be to add a function named something like
> > > > removeAll. Makes me realize that replace should maybe be named replaceAll.
> > > 
> > > Agree with both these.
> > 
> > Same. Based on Darin's earlier feedback that it'd be nice if String didn't
> > have functions that modify the String itself though, maybe free functions
> > like:
> > ```
> > makeStringByReplacingAll(const String&, UChar, ASCIILiteral/StringView/UChar)
> > makeStringByRemovingAll(const String&, UChar)
> > ```
> > 
> > It'd be consistent with the makeStringByInserting() that I recently added.
> 
> Another option would be making the member functions const and have them
> return a new WARN_UNUSED_RETURN String. If we did that, we could consider
> names like:
> 
> WARN_UNUSED_RETURN String stringByReplacingAll(UChar,
> ASCIILiteral/StringView/UChar) const
> WARN_UNUSED_RETURN String stringByRemovingAll(UChar) const

The const member function with a WARN_UNUSED_RETURN is exactly what I suggested on the previous bug when we were updating insert().
Darin is the one who suggested a free function instead (and we went with makeStringByInserting()).

I do not feel strongly either way but Darin had a preference for the free function. We should be consistent here.

[Sam Weinig]

I also don’t have a strong preference here, so I agree.

[Darin Adler]

The member functions have some advantages, easy to spot when looking over "what the String class has to offer", and a more-economical way to write these at the call sites that free functions. But I see at least two subtle advantages to the free functions (some of which would also apply to static member functions):

One is that for the parameter type, instead of "this" we can often take a StringView, so a single function can operate on an existing String but even on a string literal or a buffer of UChar, and typically with better syntax than what we’d need if it was a StringView member function (I am not fond of "StringView { }."). We can always overload for specific types (String, or String&, or String&&) if we want a more efficient variant.

Second is that the free functions can be organized logically, even put in separate headers, making String itself less of a single enormous class. We don’t have that kind of flexibility for member functions. The free functions dovetail nicely with the design of other functions, including ones that operate on more than one string like makeString itself.

For certain "transform single character" cases, in addition to functions we might make some kind of manipulator object that works with the StringConcatenate mechanism. Not sure what the right names for those are. They could do some common operations in place, like ASCII lowercasing and uppercasing, without requiring any additional memory allocation.

[Chris Dumez]

(In reply to Darin Adler from comment #17)
> The member functions have some advantages, easy to spot when looking over
> "what the String class has to offer", and a more-economical way to write
> these at the call sites that free functions. But I see at least two subtle
> advantages to the free functions (some of which would also apply to static
> member functions):
> 
> One is that for the parameter type, instead of "this" we can often take a
> StringView, so a single function can operate on an existing String but even
> on a string literal or a buffer of UChar, and typically with better syntax
> than what we’d need if it was a StringView member function (I am not fond of
> "StringView { }."). We can always overload for specific types (String, or
> String&, or String&&) if we want a more efficient variant.
> 
> Second is that the free functions can be organized logically, even put in
> separate headers, making String itself less of a single enormous class. We
> don’t have that kind of flexibility for member functions. The free functions
> dovetail nicely with the design of other functions, including ones that
> operate on more than one string like makeString itself.
> 
> For certain "transform single character" cases, in addition to functions we
> might make some kind of manipulator object that works with the
> StringConcatenate mechanism. Not sure what the right names for those are.
> They could do some common operations in place, like ASCII lowercasing and
> uppercasing, without requiring any additional memory allocation.

Note that in this patch, we deleted the overload taking a "const char*" and it was useful to find callers that could be optimized.

How can we achieve this we free functions? I guess we could declare but not define the overload taking a `const char*` but this would lead to linking errors which is not great. Do you have a good proposal for this?

[Darin Adler]

(In reply to Chris Dumez from comment #18)
> Note that in this patch, we deleted the overload taking a "const char*" and
> it was useful to find callers that could be optimized.
> 
> How can we achieve this we free functions? I guess we could declare but not
> define the overload taking a `const char*` but this would lead to linking
> errors which is not great. Do you have a good proposal for this?

Free functions can be deleted just like member functions.

[Chris Dumez]

(In reply to Darin Adler from comment #19)
> (In reply to Chris Dumez from comment #18)
> > Note that in this patch, we deleted the overload taking a "const char*" and
> > it was useful to find callers that could be optimized.
> > 
> > How can we achieve this we free functions? I guess we could declare but not
> > define the overload taking a `const char*` but this would lead to linking
> > errors which is not great. Do you have a good proposal for this?
> 
> Free functions can be deleted just like member functions.

Today I Learn :)

Sounds good to me then.