RESOLVED FIXED 237967
Clarify code for logical-to-physical mappings, and add physical-to-logical mappings 
https://bugs.webkit.org/show_bug.cgi?id=237967
Summary Clarify code for logical-to-physical mappings, and add physical-to-logical ma...
Oriol Brufau
Reported 2022-03-16 09:30:19 PDT
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.
Attachments
Patch (12.22 KB, patch)
2022-03-16 10:19 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Patch (23.91 KB, patch)
2022-03-16 18:44 PDT, Oriol Brufau 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Oriol Brufau
Comment 1 2022-03-16 10:19:03 PDT
Created attachment 454853 [details] Patch
Darin Adler
Comment 2 2022-03-16 15:42:53 PDT
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.
Oriol Brufau
Comment 3 2022-03-16 16:01:43 PDT
(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.
Oriol Brufau
Comment 4 2022-03-16 18:44:59 PDT
Created attachment 454923 [details] Patch
Oriol Brufau
Comment 5 2022-03-16 18:50:22 PDT
(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.
Darin Adler
Comment 6 2022-03-17 07:38:40 PDT
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.
EWS
Comment 7 2022-03-17 08:14:27 PDT
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].
Radar WebKit Bug Importer
Comment 8 2022-03-17 08:15:17 PDT
<rdar://problem/90431868>
Note You need to log in before you can comment on or make changes to this bug.