Bug 230058 - Use std::not_fn instead of lambda in URLParser
Summary: Use std::not_fn instead of lambda in URLParser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-08 12:35 PDT by Alex Christensen
Modified: 2021-09-10 16:52 PDT (History)
11 users (show)

See Also:


Attachments
Patch (7.17 KB, patch)
2021-09-08 12:48 PDT, Alex Christensen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (9.51 KB, patch)
2021-09-08 13:24 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (9.66 KB, patch)
2021-09-08 16:35 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (12.72 KB, patch)
2021-09-09 20:49 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (9.35 KB, patch)
2021-09-10 00:21 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (9.51 KB, patch)
2021-09-10 09:03 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (10.65 KB, patch)
2021-09-10 15:37 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2021-09-08 12:35:20 PDT
Use std::not_fn instead of lambda in URLParser
Comment 1 Alex Christensen 2021-09-08 12:48:13 PDT
Created attachment 437657 [details]
Patch
Comment 2 Alex Christensen 2021-09-08 13:24:40 PDT
Created attachment 437662 [details]
Patch
Comment 3 Alex Christensen 2021-09-08 16:35:41 PDT
Created attachment 437678 [details]
Patch
Comment 4 Fujii Hironori 2021-09-09 16:58:06 PDT
Comment on attachment 437678 [details]
Patch

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

> Source/WTF/wtf/text/StringCommon.h:539
> +template<typename CharacterType, std::enable_if_t<std::is_integral<CharacterType>::value>* = nullptr>

You can use std::is_integral_v.

> Source/WTF/wtf/text/StringImpl.h:596
> +template<typename CodeUnitMatchFunction, std::enable_if_t<std::is_invocable_r<bool, CodeUnitMatchFunction, UChar>::value>* = nullptr>

UChar → LChar
You can use is_invocable_r_v.
Comment 5 Alex Christensen 2021-09-09 20:47:38 PDT
Comment on attachment 437678 [details]
Patch

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

>> Source/WTF/wtf/text/StringImpl.h:596
>> +template<typename CodeUnitMatchFunction, std::enable_if_t<std::is_invocable_r<bool, CodeUnitMatchFunction, UChar>::value>* = nullptr>
> 
> UChar → LChar
> You can use is_invocable_r_v.

UChar is correct here because CodeUnitMatchFunction used to be bool (*)(UChar) and it will be called with UChar.
I like is_invocable_r_v though.
Comment 6 Alex Christensen 2021-09-09 20:49:41 PDT
Created attachment 437831 [details]
Patch
Comment 7 Fujii Hironori 2021-09-09 20:55:17 PDT
Comment on attachment 437831 [details]
Patch

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

> Source/WTF/wtf/text/WTFString.h:173
>      size_t find(CodeUnitMatchFunction matchFunction, unsigned start = 0) const { return m_impl ? m_impl->find(matchFunction, start) : notFound; }

Does this copy the matchFunction object? Should you use CodeUnitMatchFunction&& and std::forward here?
You don't templatize StringImpl::find(CodeUnitMatchFunction, unsigned index). Why can you call it with matchFunction? Does this convert matchFunction to bool (*)(UChar)? If so, why do you need to templatize this method?
Comment 8 Fujii Hironori 2021-09-09 21:07:35 PDT
Comment on attachment 437831 [details]
Patch

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

> Source/WTF/wtf/text/StringImpl.h:659
> +template<typename CodeUnitMatchFunction, std::enable_if_t<std::is_invocable_r_v<bool, CodeUnitMatchFunction, UChar>>*>

Now both 'find' are template functions. How about unifying them by templatize LChar and UChar?
Comment 9 Fujii Hironori 2021-09-09 21:14:47 PDT
Comment on attachment 437831 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/EventAttribution.mm:78
> +static TestNavigationDelegate *delegateAllowingAllTLS()

These changes seems came from other bug.
Comment 10 Alex Christensen 2021-09-10 00:09:15 PDT
Oh I see.  LChar is indeed correct in the one you suggested because it will be called with LChar.
Comment 11 Alex Christensen 2021-09-10 00:21:55 PDT
Created attachment 437848 [details]
Patch
Comment 12 Fujii Hironori 2021-09-10 00:44:04 PDT
Comment on attachment 437848 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Add a few explicit template parameters needed since we are passing a template parameter now instead of a function pointer.

You don't modify String::find in this patch. You don't need this change.

> Source/WTF/wtf/text/StringImpl.h:647
> +inline size_t find(const CodeUnit* characters, unsigned length, CodeUnitMatchFunction matchFunction, unsigned index)

I think it should be CodeUnitMatchFunction&&. Otherwide the callable object is coped.
Comment 13 Fujii Hironori 2021-09-10 05:15:30 PDT
Comment on attachment 437848 [details]
Patch

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

>> Source/WTF/wtf/text/StringImpl.h:647
>> +inline size_t find(const CodeUnit* characters, unsigned length, CodeUnitMatchFunction matchFunction, unsigned index)
> 
> I think it should be CodeUnitMatchFunction&&. Otherwide the callable object is coped.

It's so-called Universal Reference.
Comment 14 Alex Christensen 2021-09-10 09:03:42 PDT
Created attachment 437877 [details]
Patch
Comment 15 Fujii Hironori 2021-09-10 14:24:21 PDT
Comment on attachment 437877 [details]
Patch

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

> Source/WTF/wtf/text/StringImpl.h:647
> +inline size_t find(const CodeUnit* characters, unsigned length, CodeUnitMatchFunction&& matchFunction, unsigned index)

'find' functions in StringCommon.h and StringImpl.h are similar. I think they should be in the same file.

> Source/WebCore/html/DOMTokenList.cpp:47
> +    return token.find(isHTMLSpace<UChar>) != notFound;

You don't modify String::find in this patch. Why do you need this change?
Comment 16 Alex Christensen 2021-09-10 14:25:33 PDT
It used to be able to infer <UChar> because it was being passed as a bool(*)(UChar) but now it can't because it's a templatized invocable.
Comment 17 Fujii Hironori 2021-09-10 15:13:32 PDT
Comment on attachment 437877 [details]
Patch

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

>> Source/WebCore/html/DOMTokenList.cpp:47
>> +    return token.find(isHTMLSpace<UChar>) != notFound;
> 
> You don't modify String::find in this patch. Why do you need this change?

I confirmed that this patch can compile without this change.
Comment 18 Darin Adler 2021-09-10 15:28:23 PDT
Comment on attachment 437877 [details]
Patch

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

> Source/WTF/wtf/URLParser.cpp:2710
> +            return segment.find(std::not_fn(isASCIIHexDigit<UChar>), 2) == notFound;

find == notFound is harder to read than !contains; can we add/use contains instead?
Comment 19 Alex Christensen 2021-09-10 15:37:07 PDT
Created attachment 437916 [details]
Patch
Comment 20 EWS 2021-09-10 16:51:54 PDT
Committed r282303 (241575@main): <https://commits.webkit.org/241575@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437916 [details].
Comment 21 Radar WebKit Bug Importer 2021-09-10 16:52:27 PDT
<rdar://problem/82995709>