Bug 237967

Created attachment 454853 [details]
Patch

Comment on attachment 454853 [details]
Patch

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

Can we make a test suite for these functions using static_assert? It can be in a .cpp file so we don’t run it more than once.

> Source/WebCore/css/makeprop.pl:746
> +    const TextFlow& textflow = makeTextFlow(writingMode, direction);

Can this use auto& or auto?

> Source/WebCore/css/makeprop.pl:781
> +            print GPERF "static_assert(" . $assert . ", \"" . $assert . "\");\n";

There’s no need to repeat the assertion expression twice. Can just static_assert(expression). Earlier versions of C++ required a string every time, but the latest does not, and there’s no value to repeating the expression in string form.

> Source/WebCore/css/makeprop.pl:785
> +            my $assert =  $logicalToPhysical . "(" . $textFlow . ", " . $physicalToLogical . "(" . $textFlow . ", " . $physicalEnum . ")) == " . $physicalEnum;

Extra space here after "=".

> Source/WebCore/css/makeprop.pl:786
> +            print GPERF "static_assert(" . $assert . ", \"" . $assert . "\");\n";

Ditto.

> Source/WebCore/platform/text/WritingMode.h:160
>  constexpr inline BoxSide mapLogicalSideToPhysicalSide(TextFlow textflow, LogicalBoxSide logicalSide)

It’s never necessary to write "constexpr inline". Just "constexpr" means the same thing, for a function.

(In reply to Darin Adler from comment #2)
> Comment on attachment 454853 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454853&action=review
> 
> Can we make a test suite for these functions using static_assert? It can be
> in a .cpp file so we don’t run it more than once.

I would have written a unit test, but at first I didn't find any existing example.
I guess I have found them in Tools/TestWebKitAPI/Tests/WebCore/, do you mean something like that?

> > Source/WebCore/css/makeprop.pl:781
> > +            print GPERF "static_assert(" . $assert . ", \"" . $assert . "\");\n";
> 
> There’s no need to repeat the assertion expression twice. Can just
> static_assert(expression). Earlier versions of C++ required a string every
> time, but the latest does not, and there’s no value to repeating the
> expression in string form.

I did it in order to get a meaningful message if the assert fails.
But it doesn't matter if I write a unit test instead.

Created attachment 454923 [details]
Patch

(In reply to Darin Adler from comment #2)
> Can we make a test suite for these functions using static_assert? It can be
> in a .cpp file so we don’t run it more than once.

Added Tools/TestWebKitAPI/Tests/WebCore/WritingModeTests.cpp
Not sure if you need to take a look, or if your previous approval still holds.

> > Source/WebCore/css/makeprop.pl:746
> > +    const TextFlow& textflow = makeTextFlow(writingMode, direction);
> 
> Can this use auto& or auto?

Done.

> > Source/WebCore/css/makeprop.pl:781
> > +            print GPERF "static_assert(" . $assert . ", \"" . $assert . "\");\n";
> 
> There’s no need to repeat the assertion expression twice. Can just
> static_assert(expression). Earlier versions of C++ required a string every
> time, but the latest does not, and there’s no value to repeating the
> expression in string form.
> 
> > Source/WebCore/css/makeprop.pl:785
> > +            my $assert =  $logicalToPhysical . "(" . $textFlow . ", " . $physicalToLogical . "(" . $textFlow . ", " . $physicalEnum . ")) == " . $physicalEnum;
> 
> Extra space here after "=".
> 
> > Source/WebCore/css/makeprop.pl:786
> > +            print GPERF "static_assert(" . $assert . ", \"" . $assert . "\");\n";
> 
> Ditto.

Removed this code.

> > Source/WebCore/platform/text/WritingMode.h:160
> >  constexpr inline BoxSide mapLogicalSideToPhysicalSide(TextFlow textflow, LogicalBoxSide logicalSide)
> 
> It’s never necessary to write "constexpr inline". Just "constexpr" means the
> same thing, for a function.

Done.

Comment on attachment 454923 [details]
Patch

This is all good. I think at least some of the unit tests could be restructured so they run at compile time rather than runtime since this is largely constexpr code. But that does not have to be done just to land this.

Committed r291407 (248537@main): <https://commits.webkit.org/248537@main>

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

<rdar://problem/90431868>