Make unsigned LayoutUnit constructors explicit
Created attachment 355205 [details] Patch
Created attachment 355215 [details] Patch
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.
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.
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
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.
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
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.
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
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.
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
Created attachment 355317 [details] Patch
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.
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
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.
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
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.
Created attachment 355445 [details] Patch
Created attachment 355537 [details] Patch
Created attachment 355553 [details] Patch
Created attachment 355556 [details] Patch
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(...), ... }
(Alternatively, if the consensus is that this isn't really an improvement, then please let me know and I will happily step aside. 😅)
(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. 😂)
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.
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. :)
I think this is a good change, but haven't had a chance to review it. I'd like to hear from 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. Cool! Sorry for assuming prematurely. 😅
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)
(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.
(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.
Created attachment 358535 [details] Patch
Any further thoughts here perchance?
Ping? Happy to give this another rebase if there's still interest.
r=me
Created attachment 370273 [details] Patch for landing
Created attachment 370288 [details] Patch for landing
Comment on attachment 370288 [details] Patch for landing Clearing flags on attachment: 370288 Committed r245543: <https://trac.webkit.org/changeset/245543>
All reviewed patches have been landed. Closing bug.
<rdar://problem/50969679>