Created attachment 354535[details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 354536[details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 354537[details]
Archive of layout-test-results from ews114 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 354540[details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 354543[details]
Archive of layout-test-results from ews116 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 354544[details]
Archive of layout-test-results from ews202 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Created attachment 354549[details]
Archive of layout-test-results from ews104 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
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. :)
Created attachment 354621[details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
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.
(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.
(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 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.
(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.
> 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 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.
2018-11-11 20:55 PST, Ross Kirsling
2018-11-11 23:15 PST, Ross Kirsling
2018-11-11 23:41 PST, Ross Kirsling
2018-11-12 00:43 PST, EWS Watchlist
2018-11-12 01:39 PST, EWS Watchlist
2018-11-12 01:46 PST, EWS Watchlist
2018-11-12 03:01 PST, EWS Watchlist
2018-11-12 03:49 PST, EWS Watchlist
2018-11-12 04:04 PST, EWS Watchlist
2018-11-12 04:55 PST, EWS Watchlist
2018-11-12 16:53 PST, Ross Kirsling
2018-11-12 18:44 PST, EWS Watchlist
2018-11-12 20:29 PST, Ross Kirsling
2018-11-13 10:08 PST, Ross Kirsling
2018-11-13 11:13 PST, Ross Kirsling