Bug 191811 – Make lossy LayoutUnit constructors explicit

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

Make unsigned LayoutUnit constructors explicit

[Ross Kirsling]

Created attachment 355205 [details]
Patch

[Ross Kirsling]

Created attachment 355215 [details]
Patch

[Ross Kirsling]

Turns out this patch is relatively small, as long as we leave LayoutUnit(int) implicit.

If this is acceptable, then I think bug 191538 can simply be closed.

[EWS Watchlist]

Comment on attachment 355215 [details]
Patch

Attachment 355215 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10049860

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 355219 [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

[EWS Watchlist]

Comment on attachment 355215 [details]
Patch

Attachment 355215 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10049861

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 355220 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 355215 [details]
Patch

Attachment 355215 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10049818

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 355221 [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

[EWS Watchlist]

Comment on attachment 355215 [details]
Patch

Attachment 355215 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10049969

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 355222 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Ross Kirsling]

Created attachment 355317 [details]
Patch

[EWS Watchlist]

Comment on attachment 355317 [details]
Patch

Attachment 355317 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10085371

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 355318 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 355317 [details]
Patch

Attachment 355317 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10085374

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 355321 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Ross Kirsling]

It seems I was a little confused earlier.

Although there are few cases of implicit LayoutUnit(unsigned) and LayoutUnit(double) in use, there are hundreds of cases of implicit LayoutUnit(float). The misleading part was that we can successfully compile without addressing them (since floats will end up using the implicit LayoutUnit(int) constructor), but naturally this has unpredictable effects on runtime behavior.

So to handle this properly without a decrease in ergonomics, we still need all of those templates, and definitely will want to introduce that "0_lu" literal first.

[Ross Kirsling]

Created attachment 355445 [details]
Patch

[Ross Kirsling]

Created attachment 355537 [details]
Patch

[Ross Kirsling]

Created attachment 355553 [details]
Patch

[Ross Kirsling]

Created attachment 355556 [details]
Patch

[Ross Kirsling]

The new title here is more accurate description of the sole benefit I can see to this: to ensure that conversions from float/double don't just magically happen.

We can do this by adding either templates or float overloads to LayoutPoint, LayoutSize, LayoutRect, and the like.
This patch demonstrates the former; the latter would be a somewhat larger patch.
Templates might be more opaque but they do support double, say, at the same time.

This patch is now just a third of the size of the original; I'm hoping it's reviewable as is, mainly since I'm not sure if anything in the patch would be desired independent of the motivation to make LayoutUnit explicit.

Outside of the templates, every change should involve constructing or converting a LayoutUnit lvalue from a float-valued expression, e.g.:
- LayoutUnit foo = ...;          => LayoutUnit foo { ... };
- return ...;                    => return LayoutUnit(...);
- std::max<LayoutUnit>(..., ...) => std::max(LayoutUnit(...), ...)
- LayoutPoint(..., ...)          => { LayoutUnit(...), ... }

[Ross Kirsling]

(Alternatively, if the consensus is that this isn't really an improvement, then please let me know and I will happily step aside. 😅)

[Ross Kirsling]

(In reply to Ross Kirsling from comment #22)
> We can do this by adding either templates or float overloads to LayoutPoint,
> LayoutSize, LayoutRect, and the like.
> This patch demonstrates the former; the latter would be a somewhat larger
> patch.
> Templates might be more opaque but they do support double, say, at the same
> time.

Sorry, my earlier explanation is kind of insufficient.

The real reason float overloads would be a larger patch and potentially undesirable going forward is because of cases like this:
> LayoutPoint(someLayoutUnit(), someInt())

The intention would be to have the int get implicitly converted to LayoutUnit (and use LayoutPoint(LayoutUnit, LayoutUnit)), but the compiler complains that it's actually ambiguous, since perhaps we wanted both parameters to be implicitly converted to float (and use LayoutPoint(float, float)).

Hence this effort really probably only makes sense with the template approach.

---

Furthermore, this effort doesn't work if we mark *everything but* LayoutUnit(int) as explicit, for the reason I mentioned in comment 17 -- if you forget to explicitly convert float to LayoutUnit, you won't get an error as desired; you'll just end up using implicit LayoutUnit(int) without realizing it, which is a strictly worse situation.

Given that we don't want to make LayoutUnit(int) explicit, since that particular implicit conversion is clearly a feature, then the best we could really do is *just* mark the lossy constructors as explicit (i.e. at least float and double; debatably also unsigned long [long], though they're unused anyway). That way, when you forget to convert float to LayoutUnit, the compiler asks "were you intending to use LayoutUnit(int) or LayoutUnit(unsigned) or...?"

And that's precisely the patch here.

---

Whether implicit lossy conversion from floating-point types is enough of a concern to justify all this...is a question for the reviewers.

(I kind of wish I could've reached that explanation without putting in the level of time and effort that I did, but hey, it's been educational. 😂)

[Ross Kirsling]

Any chance for a review here? :)

It may suffice to read comments 22 through 24; the code only needs to be read if the approach is deemed desirable.

[Ross Kirsling]

I'm guessing that the unspoken consensus here is a moderate preference for the status quo?

I'll tentatively plan to close this as WONTFIX at the end of the week unless I hear otherwise. :)

[Simon Fraser (smfr)]

I think this is a good change, but haven't had a chance to review it. I'd like to hear from Zalan.

[Ross Kirsling]

(In reply to Simon Fraser (smfr) from comment #27)
> I think this is a good change, but haven't had a chance to review it. I'd
> like to hear from Zalan.

Cool! Sorry for assuming prematurely. 😅

[Konstantin Tokarev]

If we want to make float to integer conversion explicit in terms of floor/ceil/round/truncate, I think we should have ctor LayoutUnit(floor, ConversionPolicy) here and no operator=(floor)

[zalan]

(In reply to Simon Fraser (smfr) from comment #27)
> I think this is a good change, but haven't had a chance to review it. I'd
> like to hear from Zalan.
I've been wanting to do this but never had a chance (and didn't have a compelling reason to introduce such an intrusive change). I don't mind landing this.

[Ross Kirsling]

(In reply to Konstantin Tokarev from comment #29)
> If we want to make float to integer conversion explicit in terms of
> floor/ceil/round/truncate, I think we should have ctor LayoutUnit(floor,
> ConversionPolicy) here and no operator=(floor)

There are a couple of cons to this approach:

1. All of LayoutUnit's constructors clamp to int, so it seems quite natural/unambiguous that operator=(float) would follow suit.

2. The introduction of operator=(float) was aiming to reduce both the size of the diff and the burden of using these explicit constructors -- in particular, there are about 120 existing locations that would need to be updated.

(In reply to zalan from comment #30)
> (In reply to Simon Fraser (smfr) from comment #27)
> > I think this is a good change, but haven't had a chance to review it. I'd
> > like to hear from Zalan.
> I've been wanting to do this but never had a chance (and didn't have a
> compelling reason to introduce such an intrusive change). I don't mind
> landing this.

Thanks Zalan!

I'll upload a rebased version of the current patch.

[Ross Kirsling]

Created attachment 358535 [details]
Patch

[Ross Kirsling]

Any further thoughts here perchance?

[Ross Kirsling]

Ping?

Happy to give this another rebase if there's still interest.

[Antti Koivisto]

r=me

[Ross Kirsling]

Created attachment 370273 [details]
Patch for landing

[Ross Kirsling]

Created attachment 370288 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 370288 [details]
Patch for landing

Clearing flags on attachment: 370288

Committed r245543: <https://trac.webkit.org/changeset/245543>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50969679>