Bug 230058 – Use std::not_fn instead of lambda in URLParser

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Use std::not_fn instead of lambda in URLParser

[Alex Christensen]

Created attachment 437657 [details]
Patch

[Alex Christensen]

Created attachment 437662 [details]
Patch

[Alex Christensen]

Created attachment 437678 [details]
Patch

[Fujii Hironori]

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.

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 437831 [details]
Patch

[Fujii Hironori]

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?

[Fujii Hironori]

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?

[Fujii Hironori]

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.

[Alex Christensen]

Oh I see.  LChar is indeed correct in the one you suggested because it will be called with LChar.

[Alex Christensen]

Created attachment 437848 [details]
Patch

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Alex Christensen]

Created attachment 437877 [details]
Patch

[Fujii Hironori]

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?

[Alex Christensen]

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.

[Fujii Hironori]

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.

[Darin Adler]

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?

[Alex Christensen]

Created attachment 437916 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/82995709>