Bug 239500 - Replace String::replaceWithLiteral() with a String::replace() overload that takes in an ASCIILiteral
Summary: Replace String::replaceWithLiteral() with a String::replace() overload that t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-19 08:38 PDT by Chris Dumez
Modified: 2022-04-23 10:45 PDT (History)
24 users (show)

See Also:


Attachments
Patch (18.17 KB, patch)
2022-04-19 08:43 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (18.12 KB, patch)
2022-04-19 11:33 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.36 KB, patch)
2022-04-19 13:14 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (23.87 KB, patch)
2022-04-19 13:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (25.50 KB, patch)
2022-04-19 16:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2022-04-19 08:38:51 PDT
Replace String::replaceWithLiteral() with a String::replace() overload that takes in an ASCIILiteral.
Comment 1 Chris Dumez 2022-04-19 08:43:01 PDT
Created attachment 457897 [details]
Patch
Comment 2 Darin Adler 2022-04-19 10:44:55 PDT
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).
Comment 3 Chris Dumez 2022-04-19 11:33:14 PDT
Created attachment 457920 [details]
Patch
Comment 4 Sam Weinig 2022-04-19 12:29:28 PDT
(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!
Comment 5 Sam Weinig 2022-04-19 12:31:35 PDT
Should we = delete the overload that takes a const char* to ensure people aren't passing in a literal the other way?
Comment 6 Chris Dumez 2022-04-19 12:37:46 PDT
(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.
Comment 7 Chris Dumez 2022-04-19 12:46:22 PDT
(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 :)
Comment 8 Chris Dumez 2022-04-19 12:51:55 PDT
(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.
Comment 9 Chris Dumez 2022-04-19 13:14:47 PDT
Created attachment 457931 [details]
Patch
Comment 10 Chris Dumez 2022-04-19 13:36:05 PDT
Created attachment 457934 [details]
Patch
Comment 11 Chris Dumez 2022-04-19 16:52:18 PDT
Created attachment 457943 [details]
Patch
Comment 12 EWS 2022-04-19 19:33:53 PDT
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].
Comment 13 Radar WebKit Bug Importer 2022-04-19 19:34:14 PDT
<rdar://problem/92001180>
Comment 14 Sam Weinig 2022-04-20 10:59:30 PDT
(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
Comment 15 Chris Dumez 2022-04-21 18:47:11 PDT
(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.
Comment 16 Sam Weinig 2022-04-22 09:17:43 PDT
I also don’t have a strong preference here, so I agree.
Comment 17 Darin Adler 2022-04-22 10:54:33 PDT
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.
Comment 18 Chris Dumez 2022-04-22 10:59:18 PDT
(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?
Comment 19 Darin Adler 2022-04-22 11:04:01 PDT
(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.
Comment 20 Chris Dumez 2022-04-22 11:04:49 PDT
(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.