RESOLVED WONTFIX 191538
Make LayoutUnit constructors explicit. 
https://bugs.webkit.org/show_bug.cgi?id=191538
Summary Make LayoutUnit constructors explicit.
Ross Kirsling
Reported 2018-11-11 20:35:59 PST
Make LayoutUnit constructors explicit.
Attachments
Patch (484.81 KB, patch)
2018-11-11 20:55 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch (486.54 KB, patch)
2018-11-11 23:15 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch (484.60 KB, patch)
2018-11-11 23:41 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews101 for mac-sierra (3.24 MB, application/zip)
2018-11-12 00:43 PST, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (3.58 MB, application/zip)
2018-11-12 01:39 PST, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews114 for mac-sierra (4.14 MB, application/zip)
2018-11-12 01:46 PST, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews206 for win-future (13.16 MB, application/zip)
2018-11-12 03:01 PST, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews116 for mac-sierra (4.14 MB, application/zip)
2018-11-12 03:49 PST, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews202 for win-future (13.27 MB, application/zip)
2018-11-12 04:04 PST, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.84 MB, application/zip)
2018-11-12 04:55 PST, EWS Watchlist 		no flags
Details
Patch (484.76 KB, patch)
2018-11-12 16:53 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (3.46 MB, application/zip)
2018-11-12 18:44 PST, EWS Watchlist 		no flags
Details
Patch (416.73 KB, patch)
2018-11-12 20:29 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch (417.49 KB, patch)
2018-11-13 10:08 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Patch (417.13 KB, patch)
2018-11-13 11:13 PST, Ross Kirsling 		no flags
DetailsFormatted DiffDiff
Show Obsolete (14) View All Add attachment proposed patch, testcase, etc.
Ross Kirsling
Comment 1 2018-11-11 20:55:50 PST Comment hidden (obsolete)
Ross Kirsling
Comment 2 2018-11-11 23:15:25 PST Comment hidden (obsolete)
Ross Kirsling
Comment 3 2018-11-11 23:41:02 PST Comment hidden (obsolete)
EWS Watchlist
Comment 4 2018-11-12 00:43:04 PST Comment hidden (obsolete)
EWS Watchlist
Comment 5 2018-11-12 00:43:05 PST Comment hidden (obsolete)
EWS Watchlist
Comment 6 2018-11-12 01:39:20 PST Comment hidden (obsolete)
EWS Watchlist
Comment 7 2018-11-12 01:39:21 PST Comment hidden (obsolete)
EWS Watchlist
Comment 8 2018-11-12 01:46:49 PST Comment hidden (obsolete)
EWS Watchlist
Comment 9 2018-11-12 01:46:50 PST Comment hidden (obsolete)
EWS Watchlist
Comment 10 2018-11-12 03:00:55 PST Comment hidden (obsolete)
EWS Watchlist
Comment 11 2018-11-12 03:01:07 PST Comment hidden (obsolete)
EWS Watchlist
Comment 12 2018-11-12 03:49:44 PST Comment hidden (obsolete)
EWS Watchlist
Comment 13 2018-11-12 03:49:46 PST Comment hidden (obsolete)
EWS Watchlist
Comment 14 2018-11-12 04:03:50 PST Comment hidden (obsolete)
EWS Watchlist
Comment 15 2018-11-12 04:04:02 PST Comment hidden (obsolete)
EWS Watchlist
Comment 16 2018-11-12 04:55:36 PST Comment hidden (obsolete)
EWS Watchlist
Comment 17 2018-11-12 04:55:38 PST Comment hidden (obsolete)
Ross Kirsling
Comment 18 2018-11-12 16:53:29 PST Comment hidden (obsolete)
Ross Kirsling
Comment 19 2018-11-12 16:56:42 PST
This is a modernization task which fixes bug 189467 as a side effect. Not sure if the style here is desirable or not, but hoping that the patch isn't offensive in concept. :)
EWS Watchlist
Comment 20 2018-11-12 18:44:00 PST Comment hidden (obsolete)
EWS Watchlist
Comment 21 2018-11-12 18:44:01 PST Comment hidden (obsolete)
Ross Kirsling
Comment 22 2018-11-12 20:29:41 PST
Created attachment 354637 [details] Patch
Ross Kirsling
Comment 23 2018-11-12 23:18:28 PST
Hmm, I'm completely unable to reproduce the alleged linking error on my (Mojave) machine: > Undefined symbols for architecture x86_64: > "WebCore::TableLayout::tableMaxWidth", referenced from: > WebCore::FixedTableLayout::applyPreferredLogicalWidthQuirks(WebCore::LayoutUnit&, WebCore::LayoutUnit&) const in UnifiedSource371.o This patch doesn't touch that method or constant, so it seems like yet another unified sources switcheroo, except that FixedTableLayout is not missing a TableLayout include...? This should been all green now otherwise, so I suppose I'll just add reviewers and hope the situation clears up in the morning.
Darin Adler
Comment 24 2018-11-13 09:15:42 PST
(In reply to Ross Kirsling from comment #23) > Hmm, I'm completely unable to reproduce the alleged linking error on my > (Mojave) machine: > > Undefined symbols for architecture x86_64: > > "WebCore::TableLayout::tableMaxWidth", referenced from: > > WebCore::FixedTableLayout::applyPreferredLogicalWidthQuirks(WebCore::LayoutUnit&, WebCore::LayoutUnit&) const in UnifiedSource371.o > This patch doesn't touch that method or constant, so it seems like yet > another unified sources switcheroo, except that FixedTableLayout is not > missing a TableLayout include...? > > This should been all green now otherwise, so I suppose I'll just add > reviewers and hope the situation clears up in the morning. I don’t think we should just hope this clears itself up. I believe the issue is that if we declare a static data member that is const, we also need a definition at namespace scope without an initializer. So somewhere in a cpp file inside a namespace WebCore but not inside a class we need to write: const int TableLayout::tableMaxWidth; Since there is no TableLayout.cpp, we need to choose some other file (either AutoTableLayout.cpp or FixedTableLayout.cpp) and add it there. https://en.cppreference.com/w/cpp/language/static#Constant_static_members A more elegant solution, if all the compilers we support have sufficient C++17 support (not sure about the compiler we support on Windows), is that we could use constexpr instead of const in the static data member definition inside the class, and then we would not need the definition at namespace scope.
Ross Kirsling
Comment 25 2018-11-13 09:55:43 PST
(In reply to Darin Adler from comment #24) > (In reply to Ross Kirsling from comment #23) > > Hmm, I'm completely unable to reproduce the alleged linking error on my > > (Mojave) machine: > > > Undefined symbols for architecture x86_64: > > > "WebCore::TableLayout::tableMaxWidth", referenced from: > > > WebCore::FixedTableLayout::applyPreferredLogicalWidthQuirks(WebCore::LayoutUnit&, WebCore::LayoutUnit&) const in UnifiedSource371.o > > This patch doesn't touch that method or constant, so it seems like yet > > another unified sources switcheroo, except that FixedTableLayout is not > > missing a TableLayout include...? > > > > This should been all green now otherwise, so I suppose I'll just add > > reviewers and hope the situation clears up in the morning. > > I don’t think we should just hope this clears itself up. > > I believe the issue is that if we declare a static data member that is > const, we also need a definition at namespace scope without an initializer. > So somewhere in a cpp file inside a namespace WebCore but not inside a class > we need to write: > > const int TableLayout::tableMaxWidth; > > Since there is no TableLayout.cpp, we need to choose some other file (either > AutoTableLayout.cpp or FixedTableLayout.cpp) and add it there. > > https://en.cppreference.com/w/cpp/language/static#Constant_static_members > > A more elegant solution, if all the compilers we support have sufficient > C++17 support (not sure about the compiler we support on Windows), is that > we could use constexpr instead of const in the static data member definition > inside the class, and then we would not need the definition at namespace > scope. Thanks, that's quite helpful! WinCairo isn't complaining about constexpr there, so I'll give it a shot.
Ross Kirsling
Comment 26 2018-11-13 10:08:27 PST
Created attachment 354679 [details] Patch
Ross Kirsling
Comment 27 2018-11-13 11:07:57 PST
Argh, apparently this doesn't fix the issue. And I evidently can't repro it on High Sierra either, so perhaps it's Sierra-specific. Regardless, it seems it must be triggered by the templatized copy assignment, so if I explicitly convert to LayoutUnit here, we should be fine: https://github.com/WebKit/webkit/blob/master/Source/WebCore/rendering/FixedTableLayout.cpp#L196
Ross Kirsling
Comment 28 2018-11-13 11:13:06 PST
Created attachment 354685 [details] Patch
Ross Kirsling
Comment 29 2018-11-13 15:30:16 PST
All green at last! (The AppleWin build has been broken but I verified it locally.)
Ross Kirsling
Comment 30 2018-11-14 16:08:06 PST
Would someone have time to review this by any chance? It's large and tedious, but thankfully mostly mechanical.
Darin Adler
Comment 31 2018-11-16 09:05:29 PST
Comment on attachment 354685 [details] Patch Looks correctly done. I’m not sure this is an improvement. Did making this change catch any existing errors in the code or lead to any improvements other than fixing the build failure on Windows? I’d like to know more about what we’ll accomplish that will help the health of the project going forward with this change. Typically we make constructors explicit because we are concerned that the implicit ones hide a costly operation or an operation that might fail with some values. When introducing LayoutUnit, the fact that there was an implicit conversion *from* int was one of the intentional features. I wouldn’t necessarily reverse that design decision unless the benefit is substantial. Changing all the cases of 0 to LayoutUnit(0) or { } is a bit ugly and makes me wish for a more elegant solution. I’m sure we could come up with one. Maybe a literal for LayoutUnit(0) using a user-defined integer literal like 0_layout? Or maybe even a layoutZero() function would be more elegant? Converting functions to function templates may introduce problems with type ambiguity in the future or might improve efficiency. I can’t really tell. Aside from the decision of whether to do this at all, I also think that it would be sensible to land smaller patches with changes that are either entirely harmless or better, that make things clearer. Patches that don’t depend on the explicit change. For example, I would support a pass that removes all the "= 0" since LayoutUnit is always set to 0 by default, even aside from our decision of whether to make the explicit constructor change. In fact, almost all the changes at call sites could be landed separately before throwing the switch, and this could be done in smaller pieces that are easier to audit. It’s easier to spot mistakes in smaller patches.
Ross Kirsling
Comment 32 2018-11-16 12:58:55 PST
(In reply to Darin Adler from comment #31) > Comment on attachment 354685 [details] > Patch > > Looks correctly done. I’m not sure this is an improvement. > > Did making this change catch any existing errors in the code or lead to any > improvements other than fixing the build failure on Windows? > > I’d like to know more about what we’ll accomplish that will help the health > of the project going forward with this change. > > Typically we make constructors explicit because we are concerned that the > implicit ones hide a costly operation or an operation that might fail with > some values. When introducing LayoutUnit, the fact that there was an > implicit conversion *from* int was one of the intentional features. I > wouldn’t necessarily reverse that design decision unless the benefit is > substantial. Yeah, I figured that could well be a cause for objection. The MSVC problem was my initial motivation in the matter, but I hoped it could be framed as a modernization task -- in particular, places where a float gets implicitly converted to LayoutUnit and then subsequently operated on by another float seem to present opportunity for "one pixel off" surprises. > Changing all the cases of 0 to LayoutUnit(0) or { } is a bit ugly and makes > me wish for a more elegant solution. I’m sure we could come up with one. > Maybe a literal for LayoutUnit(0) using a user-defined integer literal like > 0_layout? Or maybe even a layoutZero() function would be more elegant? This is the primary thing I was hoping for feedback on. The literal and zero function approaches both occurred to me, but being difficult to predict what folks would prefer, I figured having a patch to complain about was the best starting point. > Converting functions to function templates may introduce problems with type > ambiguity in the future or might improve efficiency. I can’t really tell. Indeed. I think the templates are necessary for having this be usable at all (in particular, the reason this patch isn't multiple times larger in line count is thanks to those templates), but in certain cases one might wonder if the explicitness benefit is getting partially blurred out. If so, I would hope those cases might be an opportunity to re-evaluate whether certain functions that interact with each other are really typed as sensibly as they could be (as in the "now it's a float again!" situation mentioned above)... > Aside from the decision of whether to do this at all, I also think that it > would be sensible to land smaller patches with changes that are either > entirely harmless or better, that make things clearer. Patches that don’t > depend on the explicit change. > > For example, I would support a pass that removes all the "= 0" since > LayoutUnit is always set to 0 by default, even aside from our decision of > whether to make the explicit constructor change. > > In fact, almost all the changes at call sites could be landed separately > before throwing the switch, and this could be done in smaller pieces that > are easier to audit. It’s easier to spot mistakes in smaller patches. Sounds like wise advice! Perhaps I'll give that a go. Also, if the idea is that a LayoutUnit is an embellished int and ought to be coercible as such, then perhaps the other unary constructors should be made explicit first? This would seem consistent with LayoutPoint / LayoutRect / LayoutSize.
Antti Koivisto
Comment 33 2018-11-17 23:05:50 PST
> Changing all the cases of 0 to LayoutUnit(0) or { } is a bit ugly and makes > me wish for a more elegant solution. I’m sure we could come up with one. > Maybe a literal for LayoutUnit(0) using a user-defined integer literal like > 0_layout? Or maybe even a layoutZero() function would be more elegant? I think literals could be nice. 0_lu or something. It should be done first and separately though.
Ross Kirsling
Comment 34 2019-05-20 18:56:33 PDT
Closing -- bug 191811 has landed and I believe that's as far as we want to go here.
Alex Christensen
Comment 35 2021-11-01 12:46:35 PDT
Comment on attachment 354685 [details] Patch This has been requesting review for more than one year. If this is still needed, please rebase and re-request review.
Note You need to log in before you can comment on or make changes to this bug.