The current code for logical-to-physical mappings is not easy to understand. mapLogicalSideToPhysicalSide relies on casting a LogicalBoxSide enum into a BoxSide enum, and then maybe casting it into an int and back into a BoxSide... So the code relies on the arbitrary order of the enum values, and it's hard to grasp what it's doing. mapLogicalCornerToPhysicalCorner just happens to work but again it's quite difficult to follow. For example, if bug 166941 adds new writing modes, most probably the entire method will need to be changed. Also, in bug 236199 I will need physical-to-logical mappings, i.e. the inverse of the current functions. But the way that they are now, writing the inverse code is not straight-forward at all. So these functions should be refactored to be understandable, and should have inverse functions where it's clear looking at the code that they are really inverse functions.
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>