[webkit-reviews] review granted: [Bug 237967] Clarify code for logical-to-physical mappings, and add physical-to-logical mappings : [Attachment 454853] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Oriol Brufau <obrufau at igalia.com>'s
request for review:
Bug 237967: Clarify code for logical-to-physical mappings, and add
physical-to-logical mappings
https://bugs.webkit.org/show_bug.cgi?id=237967

Attachment 454853: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 454853
  --> https://bugs.webkit.org/attachment.cgi?id=454853
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.