Bug 213416 – Convert much of the SVG string parsing code to use Optional based return values rather than out-parameters

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Convert much of the SVG string parsing code to use Optional based return values rather than out-parameters

[Sam Weinig]

Created attachment 402345 [details]
Patch

[Sam Weinig]

Created attachment 402365 [details]
Patch

[Sam Weinig]

Created attachment 402369 [details]
Patch

[Sam Weinig]

Created attachment 402371 [details]
Patch

[Sam Weinig]

Created attachment 402373 [details]
Patch

[Sam Weinig]

Created attachment 402374 [details]
Patch

[Sam Weinig]

Created attachment 402376 [details]
Patch

[Sam Weinig]

Created attachment 402395 [details]
Patch

[Sam Weinig]

Created attachment 402407 [details]
Patch

[Anders Carlsson]

Comment on attachment 402407 [details]
Patch

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

> Source/WebCore/svg/SVGAnimateMotionElement.cpp:165
> +    m_toPointAtEndOfDuration = parsePoint(toAtEndOfDurationString).valueOr(FloatPoint { });
>      m_hasToPointAtEndOfDuration = true;

Looks like m_toPointAtEndOfDuration could probably be an optional as well?

> Source/WebCore/svg/SVGFEImageElement.cpp:-121
> -        preserveAspectRatio.parse(value);

What happened to this parse call?

> Source/WebCore/svg/SVGPathStringSource.cpp:299
> +    {

Does the { really go here?

[Darin Adler]

Comment on attachment 402407 [details]
Patch

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

Another one of those patches where I have a local git branch with lots of nearly identical changes. Very happy to see this done!

> Source/WebCore/svg/SVGAngleValue.cpp:109
> +    auto helper = [&](const auto* ptr, const auto* end) -> ExceptionOr<void> {

I would probably just do auto twice here.

Noticed that whether there is a space after "]" and before "(" I snot the same here as in another recent patch.

> Source/WebCore/svg/SVGAngleValue.cpp:125
> +        const auto* ptr = value.characters8();
> +        const auto* end = ptr + value.length();

Just auto maybe instead of const auto*?

Also, no need for "end" local variable.

> Source/WebCore/svg/SVGAngleValue.cpp:130
> +    const auto* ptr = value.characters16();
> +    const auto* end = ptr + value.length();

Ditto.

>> Source/WebCore/svg/SVGAnimateMotionElement.cpp:165
>>      m_hasToPointAtEndOfDuration = true;
> 
> Looks like m_toPointAtEndOfDuration could probably be an optional as well?

"true"

> Source/WebCore/svg/SVGAnimationElement.cpp:183
> -        parseKeySplines(value, m_keySplines);
> +        if (auto keySplines = parseKeySplines(value))
> +            m_keySplines = WTFMove(*keySplines);
> +        else
> +            m_keySplines.clear();

Does this preserve the old behavior?

Also, seems like some refactoring could make this a one-liner instead of an if statement. Maybe valueOr or making more things Optional?

>> Source/WebCore/svg/SVGFEImageElement.cpp:-121
>> -        preserveAspectRatio.parse(value);
> 
> What happened to this parse call?

The parsing is now part of the constructor that takes a string.

> Source/WebCore/svg/SVGFitToViewBox.cpp:74
> +        if (!value.isNull()) {
> +            if (auto result = parseViewBox(value)) {
> +                setViewBox(WTFMove(*result));
> +                return true;
> +            }
> +        }

Does the null check help? Maybe parseViewBox will relatively quickly fail when passed a null string and we don’t need this optimization.

> Source/WebCore/svg/SVGFitToViewBox.h:69
> +    Optional<FloatRect> parseViewBox(const AtomString& value);
> +    Optional<FloatRect> parseViewBox(const UChar*& start, const UChar* end, bool validate = true);

Peculiar to have a parse function take an AtomString. I suggest changing it to take a StringView. Then we would not need any overloading; callers of both of the current functions could call a new single one.

> Source/WebCore/svg/SVGHKernElement.cpp:49
> +    // FIXME: Can this be shared with SVGVKernElement::buildVerticalKerningPair

Missing question mark as the end of this question.

> Source/WebCore/svg/SVGHKernElement.cpp:84
> +    SVGKerningPair kerningPair;
> +    kerningPair.glyphName1 = WTFMove(*glyphName1);
> +    kerningPair.glyphName2 = WTFMove(*glyphName2);
> +    kerningPair.unicodeRange1 = WTFMove(unicodeString1->first);
> +    kerningPair.unicodeName1 = WTFMove(unicodeString1->second);
> +    kerningPair.unicodeRange2 = WTFMove(unicodeString2->first);
> +    kerningPair.unicodeName2 = WTFMove(unicodeString2->second);
> +    kerningPair.kerning = kerning;
> +    return kerningPair;

Is construction rather than assignment a possibility here?

> Source/WebCore/svg/SVGParserUtilities.h:57
> +Optional<float> parseNumber(const LChar*& current, const LChar* end, SuffixSkippingPolicy = SuffixSkippingPolicy::Skip);
> +Optional<float> parseNumber(const UChar*& current, const UChar* end, SuffixSkippingPolicy = SuffixSkippingPolicy::Skip);
> +Optional<float> parseNumber(const String&, SuffixSkippingPolicy = SuffixSkippingPolicy::Skip);
> +
> +Optional<std::pair<float, float>> parseNumberOptionalNumber(const String&);
> +
> +Optional<bool> parseArcFlag(const LChar*& current, const LChar* end);
> +Optional<bool> parseArcFlag(const UChar*& current, const UChar* end);
> +
> +Optional<FloatPoint> parsePoint(const String&);
> +Optional<FloatRect> parseRect(const String&);
> +
> +Optional<FloatPoint> parseFloatPoint(const LChar*& current, const LChar* end);
> +Optional<FloatPoint> parseFloatPoint(const UChar*& current, const UChar* end);
> +
> +Optional<std::pair<UnicodeRanges, HashSet<String>>> parseKerningUnicodeString(const String&);
> +Optional<HashSet<String>> parseGlyphName(const String&);

Seems like these functions should all take StringView. Except I guess the ones that "advance a pointer" can’t yet. Certainly the ones that take String should likely take a StringView unless String creates some optimization opportunity.

> Source/WebCore/svg/SVGParserUtilities.h:62
> +template<typename CharacterType> inline bool isSVGSpace(CharacterType c)

constexpr instead of inline

> Source/WebCore/svg/SVGParserUtilities.h:67
> +template<typename CharacterType> inline bool skipOptionalSVGSpaces(const CharacterType*& ptr, const CharacterType* end)

Not sure we need to say inline. It’s not like more inlining will occur with the keyword than without it.

Also, this function seems closely related to other skipUntil functions elsewhere.

> Source/WebCore/svg/SVGParserUtilities.h:74
> +template<typename CharacterType> inline bool skipOptionalSVGSpacesOrDelimiter(const CharacterType*& ptr, const CharacterType* end, char delimiter = ',')

Ditto.

> Source/WebCore/svg/SVGPathBlender.cpp:131
> +    auto parsedTo = std::invoke(function, toSource);

I think we should WTFMove the function here. Can’t safely do it above, but can do it here.

> Source/WebCore/svg/SVGVKernElement.cpp:73
> +    bool ok = false;
> +    auto kerning = attributeWithoutSynchronization(SVGNames::kAttr).string().toFloat(&ok);
> +    if (!ok)
> +        return WTF::nullopt;

I think we have an optional-based float parsing API somewhere instead of the "out argument OK boolean one". If not, future refactoring I guess.

> Source/WebCore/svg/SVGVKernElement.cpp:83
> +    SVGKerningPair kerningPair;
> +    kerningPair.glyphName1 = WTFMove(*glyphName1);
> +    kerningPair.glyphName2 = WTFMove(*glyphName2);
> +    kerningPair.unicodeRange1 = WTFMove(unicodeString1->first);
> +    kerningPair.unicodeName1 = WTFMove(unicodeString1->second);
> +    kerningPair.unicodeRange2 = WTFMove(unicodeString2->first);
> +    kerningPair.unicodeName2 = WTFMove(unicodeString2->second);
> +    kerningPair.kerning = kerning;
> +    return kerningPair;

Same question about constructing rather than assigning.

> Source/WebCore/svg/properties/SVGAnimationAdditiveValueFunctionImpl.h:258
> +        float fromNumber = parseNumber(from).valueOr(0);
> +        float toNumber = parseNumber(to).valueOr(0);

Not sure we need local variables. One liner would be nicer.

> Source/WebCore/svg/properties/SVGAnimationAdditiveValueFunctionImpl.h:259
>          return fabsf(toNumber - fromNumber);

std::abs better than fabsf.

> Source/WebCore/svg/properties/SVGPropertyTraits.h:81
> +        return std::make_pair(static_cast<int>(roundf(result->first)), static_cast<int>(roundf(result->second)));

std::round better than roundf

[Sam Weinig]

Created attachment 402431 [details]
Patch

[Sam Weinig]

(In reply to Anders Carlsson from comment #10)
> Comment on attachment 402407 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402407&action=review
> 
> > Source/WebCore/svg/SVGAnimateMotionElement.cpp:165
> > +    m_toPointAtEndOfDuration = parsePoint(toAtEndOfDurationString).valueOr(FloatPoint { });
> >      m_hasToPointAtEndOfDuration = true;
> 
> Looks like m_toPointAtEndOfDuration could probably be an optional as well?

Oh nice. Fixed.

> 
> > Source/WebCore/svg/SVGFEImageElement.cpp:-121
> > -        preserveAspectRatio.parse(value);
> 
> What happened to this parse call?

The SVGPreserveAspectRatioValue constructor calls parse.

> 
> > Source/WebCore/svg/SVGPathStringSource.cpp:299
> > +    {
> 
> Does the { really go here?

Nope. Fixed.



(In reply to Darin Adler from comment #11)
> Comment on attachment 402407 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402407&action=review
> 
> Another one of those patches where I have a local git branch with lots of
> nearly identical changes. Very happy to see this done!
> 
> > Source/WebCore/svg/SVGAngleValue.cpp:109
> > +    auto helper = [&](const auto* ptr, const auto* end) -> ExceptionOr<void> {
> 
> I would probably just do auto twice here.

Fixed.

> 
> Noticed that whether there is a space after "]" and before "(" I snot the
> same here as in another recent patch.

Oh no. Quick search of WebCore for ")[" yields 1508 results vs. 1257 for "] (". (I think they are all for lambdas, but I could be wrong. Going to pick the no space since it has more, and try to codify it in the style checker and fix the rest in a follow up. Seems silly for that not to be defined.

> 
> > Source/WebCore/svg/SVGAngleValue.cpp:125
> > +        const auto* ptr = value.characters8();
> > +        const auto* end = ptr + value.length();
> 
> Just auto maybe instead of const auto*?
> 
> Also, no need for "end" local variable.

Given the prevalence of this idiom, I really want to replace it with something nicer.

Making the generic function is easy enough:

    template<typename StringType, typename Function> decltype(auto) characters(const StringType& value, Function&& functor)
    {
        if (string.is8Bit()) {
            auto* ptr = string.characters8();
            return functor(ptr, ptr + string.length());
        } else {
            auto* ptr = string.characters16();
            return functor(cur, ptr + string.length());
        }
    }

Used as:

    characters(myString, [](auto* ptr, auto* end) {
        ...
    });

The real question is do we make a free function like that, or a member of the String family of functions? And what do we name it. Going to address this in the next patch, which will be the real reason I started on this, getting rid of unnecessary upconverts.

> 
> > Source/WebCore/svg/SVGAngleValue.cpp:130
> > +    const auto* ptr = value.characters16();
> > +    const auto* end = ptr + value.length();
> 
> Ditto.

Fixed.

> 
> >> Source/WebCore/svg/SVGAnimateMotionElement.cpp:165
> >>      m_hasToPointAtEndOfDuration = true;
> > 
> > Looks like m_toPointAtEndOfDuration could probably be an optional as well?
> 
> "true"

Yup. (Took me much longer than I would like to admit to figure out that Anders wrote the first one, and who you were talking too :). )

> 
> > Source/WebCore/svg/SVGAnimationElement.cpp:183
> > -        parseKeySplines(value, m_keySplines);
> > +        if (auto keySplines = parseKeySplines(value))
> > +            m_keySplines = WTFMove(*keySplines);
> > +        else
> > +            m_keySplines.clear();
> 
> Does this preserve the old behavior?

Yeah. The old parseKeySplines called clear() on the vector as the first thing it did when passed in.

> 
> Also, seems like some refactoring could make this a one-liner instead of an
> if statement. Maybe valueOr or making more things Optional?
> 
> >> Source/WebCore/svg/SVGFEImageElement.cpp:-121
> >> -        preserveAspectRatio.parse(value);
> > 
> > What happened to this parse call?
> 
> The parsing is now part of the constructor that takes a string.

Yup.

> 
> > Source/WebCore/svg/SVGFitToViewBox.cpp:74
> > +        if (!value.isNull()) {
> > +            if (auto result = parseViewBox(value)) {
> > +                setViewBox(WTFMove(*result));
> > +                return true;
> > +            }
> > +        }
> 
> Does the null check help? Maybe parseViewBox will relatively quickly fail
> when passed a null string and we don’t need this optimization.

I this parseViewBox will likely crash if it's null due to how UpconvertedCharacters() works. But I will double check.

> 
> > Source/WebCore/svg/SVGFitToViewBox.h:69
> > +    Optional<FloatRect> parseViewBox(const AtomString& value);
> > +    Optional<FloatRect> parseViewBox(const UChar*& start, const UChar* end, bool validate = true);
> 
> Peculiar to have a parse function take an AtomString. I suggest changing it
> to take a StringView. Then we would not need any overloading; callers of
> both of the current functions could call a new single one.

I think we would still need the (const UChar*&, ...) one for now, as the caller wants "start" advanced as part of the call.

> 
> > Source/WebCore/svg/SVGHKernElement.cpp:49
> > +    // FIXME: Can this be shared with SVGVKernElement::buildVerticalKerningPair
> 
> Missing question mark as the end of this question.

Fixed.

> 
> > Source/WebCore/svg/SVGHKernElement.cpp:84
> > +    SVGKerningPair kerningPair;
> > +    kerningPair.glyphName1 = WTFMove(*glyphName1);
> > +    kerningPair.glyphName2 = WTFMove(*glyphName2);
> > +    kerningPair.unicodeRange1 = WTFMove(unicodeString1->first);
> > +    kerningPair.unicodeName1 = WTFMove(unicodeString1->second);
> > +    kerningPair.unicodeRange2 = WTFMove(unicodeString2->first);
> > +    kerningPair.unicodeName2 = WTFMove(unicodeString2->second);
> > +    kerningPair.kerning = kerning;
> > +    return kerningPair;
> 
> Is construction rather than assignment a possibility here?

It can, done. What I really wanted here (and with the path segment types) since the order of arguments wasn't super clear as it is in other aggregates was to use designated initializer syntax (https://en.cppreference.com/w/cpp/language/aggregate_initialization#Designated_initializers) where you could do both:

    return SVGKerningPair {
        .glyphName1 = WTFMove(*glyphName1),
        .glyphName2 = WTFMove(*glyphName2),
        ...
    };

But it turns out I have to wait until c++20 for that. Alas.

> 
> > Source/WebCore/svg/SVGParserUtilities.h:57
> > +Optional<float> parseNumber(const LChar*& current, const LChar* end, SuffixSkippingPolicy = SuffixSkippingPolicy::Skip);
> > +Optional<float> parseNumber(const UChar*& current, const UChar* end, SuffixSkippingPolicy = SuffixSkippingPolicy::Skip);
> > +Optional<float> parseNumber(const String&, SuffixSkippingPolicy = SuffixSkippingPolicy::Skip);
> > +
> > +Optional<std::pair<float, float>> parseNumberOptionalNumber(const String&);
> > +
> > +Optional<bool> parseArcFlag(const LChar*& current, const LChar* end);
> > +Optional<bool> parseArcFlag(const UChar*& current, const UChar* end);
> > +
> > +Optional<FloatPoint> parsePoint(const String&);
> > +Optional<FloatRect> parseRect(const String&);
> > +
> > +Optional<FloatPoint> parseFloatPoint(const LChar*& current, const LChar* end);
> > +Optional<FloatPoint> parseFloatPoint(const UChar*& current, const UChar* end);
> > +
> > +Optional<std::pair<UnicodeRanges, HashSet<String>>> parseKerningUnicodeString(const String&);
> > +Optional<HashSet<String>> parseGlyphName(const String&);
> 
> Seems like these functions should all take StringView. Except I guess the
> ones that "advance a pointer" can’t yet. Certainly the ones that take String
> should likely take a StringView unless String creates some optimization
> opportunity.

Nope. They all just immediately create a StringView :). Fixed.

> 
> > Source/WebCore/svg/SVGParserUtilities.h:62
> > +template<typename CharacterType> inline bool isSVGSpace(CharacterType c)
> 
> constexpr instead of inline

Fixed.

> 
> > Source/WebCore/svg/SVGParserUtilities.h:67
> > +template<typename CharacterType> inline bool skipOptionalSVGSpaces(const CharacterType*& ptr, const CharacterType* end)
> 
> Not sure we need to say inline. It’s not like more inlining will occur with
> the keyword than without it.
> 
> Also, this function seems closely related to other skipUntil functions
> elsewhere.

Yeah, all these micro-parser related functions need to be grouped together in a little toolkit and shared. 

> 
> > Source/WebCore/svg/SVGParserUtilities.h:74
> > +template<typename CharacterType> inline bool skipOptionalSVGSpacesOrDelimiter(const CharacterType*& ptr, const CharacterType* end, char delimiter = ',')
> 
> Ditto.

Making it constexpr, removing the inline.

> 
> > Source/WebCore/svg/SVGPathBlender.cpp:131
> > +    auto parsedTo = std::invoke(function, toSource);
> 
> I think we should WTFMove the function here. Can’t safely do it above, but
> can do it here.

Interesting. What would be the value of that here? 

> 
> > Source/WebCore/svg/SVGVKernElement.cpp:73
> > +    bool ok = false;
> > +    auto kerning = attributeWithoutSynchronization(SVGNames::kAttr).string().toFloat(&ok);
> > +    if (!ok)
> > +        return WTF::nullopt;
> 
> I think we have an optional-based float parsing API somewhere instead of the
> "out argument OK boolean one". If not, future refactoring I guess.

Yeah, it's pretty odd that this one SVG function doesn't use the parseNumber() family of functions, but I didn't want to change that right now.

> 
> > Source/WebCore/svg/SVGVKernElement.cpp:83
> > +    SVGKerningPair kerningPair;
> > +    kerningPair.glyphName1 = WTFMove(*glyphName1);
> > +    kerningPair.glyphName2 = WTFMove(*glyphName2);
> > +    kerningPair.unicodeRange1 = WTFMove(unicodeString1->first);
> > +    kerningPair.unicodeName1 = WTFMove(unicodeString1->second);
> > +    kerningPair.unicodeRange2 = WTFMove(unicodeString2->first);
> > +    kerningPair.unicodeName2 = WTFMove(unicodeString2->second);
> > +    kerningPair.kerning = kerning;
> > +    return kerningPair;
> 
> Same question about constructing rather than assigning.

Fixed.

> 
> > Source/WebCore/svg/properties/SVGAnimationAdditiveValueFunctionImpl.h:258
> > +        float fromNumber = parseNumber(from).valueOr(0);
> > +        float toNumber = parseNumber(to).valueOr(0);
> 
> Not sure we need local variables. One liner would be nicer.

Fixed.

> 
> > Source/WebCore/svg/properties/SVGAnimationAdditiveValueFunctionImpl.h:259
> >          return fabsf(toNumber - fromNumber);
> 
> std::abs better than fabsf.

Fixed.

> 
> > Source/WebCore/svg/properties/SVGPropertyTraits.h:81
> > +        return std::make_pair(static_cast<int>(roundf(result->first)), static_cast<int>(roundf(result->second)));
> 
> std::round better than roundf

Fixed.

[Darin Adler]

Comment on attachment 402407 [details]
Patch

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

>>> Source/WebCore/svg/SVGPathBlender.cpp:131
>>> +    auto parsedTo = std::invoke(function, toSource);
>> 
>> I think we should WTFMove the function here. Can’t safely do it above, but can do it here.
> 
> Interesting. What would be the value of that here?

Not sure but std::invoke does take an rvalue reference so I assume it has some value.

[EWS]

Committed r263334: <https://trac.webkit.org/changeset/263334>

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

[Radar WebKit Bug Importer]

<rdar://problem/64579570>

[Sam Weinig]

(In reply to Darin Adler from comment #14)
> Comment on attachment 402407 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402407&action=review
> 
> >>> Source/WebCore/svg/SVGPathBlender.cpp:131
> >>> +    auto parsedTo = std::invoke(function, toSource);
> >> 
> >> I think we should WTFMove the function here. Can’t safely do it above, but can do it here.
> > 
> > Interesting. What would be the value of that here?
> 
> Not sure but std::invoke does take an rvalue reference so I assume it has
> some value.

This is one of those corners of c++ I am still wrapping my head around all these years later, but I think in this case, what invoke is taking is a forwarding (or sometimes called universal) reference, not actually an rvalue reference, which is the kind of reference that allows for so called "perfect forwarding" (meaning that the arguments to std::invoke can either be rvalue or lvalue references, and invoke just passes that through).

I get why this would make sense for other arguments to std::invoke (e.g. the arguments to the function you are invoking), but I don't quite understand the use of the function object itself being either an l-value or r-value, but I am sure there is some obscure one.