Bug 191538 - Make LayoutUnit constructors explicit.
Summary: Make LayoutUnit constructors explicit.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords:
Depends on: 191791 191811 191915
Blocks:
  Show dependency tree / graph
 
Reported: 2018-11-11 20:35 PST by Ross Kirsling
Modified: 2021-11-01 12:46 PDT (History)
7 users (show)

See Also:

Attachments
Patch (484.81 KB, patch)
2018-11-11 20:55 PST, Ross Kirsling 		no flags Details | Formatted Diff | Diff
Patch (486.54 KB, patch)
2018-11-11 23:15 PST, Ross Kirsling 		no flags Details | Formatted Diff | Diff
Patch (484.60 KB, patch)
2018-11-11 23:41 PST, Ross Kirsling 		no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Patch (417.49 KB, patch)
2018-11-13 10:08 PST, Ross Kirsling 		no flags Details | Formatted Diff | Diff
Patch (417.13 KB, patch)
2018-11-13 11:13 PST, Ross Kirsling 		no flags Details | Formatted Diff | Diff
Show Obsolete (14) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2018-11-11 20:35:59 PST 
Make LayoutUnit constructors explicit.
Comment 1 Ross Kirsling 2018-11-11 20:55:50 PST Comment hidden (obsolete)
Comment 2 Ross Kirsling 2018-11-11 23:15:25 PST Comment hidden (obsolete)
Comment 3 Ross Kirsling 2018-11-11 23:41:02 PST Comment hidden (obsolete)
Comment 4 EWS Watchlist 2018-11-12 00:43:04 PST Comment hidden (obsolete)
Comment 5 EWS Watchlist 2018-11-12 00:43:05 PST Comment hidden (obsolete)
Comment 6 EWS Watchlist 2018-11-12 01:39:20 PST Comment hidden (obsolete)
Comment 7 EWS Watchlist 2018-11-12 01:39:21 PST Comment hidden (obsolete)
Comment 8 EWS Watchlist 2018-11-12 01:46:49 PST Comment hidden (obsolete)
Comment 9 EWS Watchlist 2018-11-12 01:46:50 PST Comment hidden (obsolete)
Comment 10 EWS Watchlist 2018-11-12 03:00:55 PST Comment hidden (obsolete)
Comment 11 EWS Watchlist 2018-11-12 03:01:07 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2018-11-12 03:49:44 PST Comment hidden (obsolete)
Comment 13 EWS Watchlist 2018-11-12 03:49:46 PST Comment hidden (obsolete)
Comment 14 EWS Watchlist 2018-11-12 04:03:50 PST Comment hidden (obsolete)
Comment 15 EWS Watchlist 2018-11-12 04:04:02 PST Comment hidden (obsolete)
Comment 16 EWS Watchlist 2018-11-12 04:55:36 PST Comment hidden (obsolete)
Comment 17 EWS Watchlist 2018-11-12 04:55:38 PST Comment hidden (obsolete)
Comment 18 Ross Kirsling 2018-11-12 16:53:29 PST Comment hidden (obsolete)
Comment 19 Ross Kirsling 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. :)
Comment 20 EWS Watchlist 2018-11-12 18:44:00 PST Comment hidden (obsolete)
Comment 21 EWS Watchlist 2018-11-12 18:44:01 PST Comment hidden (obsolete)
Comment 22 Ross Kirsling 2018-11-12 20:29:41 PST 
Created attachment 354637 [details]
Patch
Comment 23 Ross Kirsling 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.
Comment 24 Darin Adler 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.
Comment 25 Ross Kirsling 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.
Comment 26 Ross Kirsling 2018-11-13 10:08:27 PST 
Created attachment 354679 [details]
Patch
Comment 27 Ross Kirsling 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
Comment 28 Ross Kirsling 2018-11-13 11:13:06 PST 
Created attachment 354685 [details]
Patch
Comment 29 Ross Kirsling 2018-11-13 15:30:16 PST 
All green at last!
(The AppleWin build has been broken but I verified it locally.)
Comment 30 Ross Kirsling 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.
Comment 31 Darin Adler 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.
Comment 32 Ross Kirsling 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.
Comment 33 Antti Koivisto 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.
Comment 34 Ross Kirsling 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.
Comment 35 Alex Christensen 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.