Bug 83848 - Support size_t multiplication and division operators on LayoutUnit
: Support size_t multiplication and division operators on LayoutUnit
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Platform
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Julien Chaffraix
:
Depends on:
Blocks: 60318 104700
  Show dependency treegraph
 
Reported: 2012-04-12 18:41 PDT by Emil A Eklund
Modified: 2013-01-07 09:27 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.90 KB, patch)
2012-04-12 18:46 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (3.88 KB, patch)
2012-04-13 11:43 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (4.00 KB, patch)
2012-04-13 13:11 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (4.11 KB, patch)
2012-04-14 18:37 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Updated patch: add overloaded operators for most unsigned types, per Darin's suggestion. (8.07 KB, patch)
2012-12-18 15:07 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Updated patch without the bad multipliciation test and with an overflowing test. (8.63 KB, patch)
2012-12-18 15:50 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Updated patch 3: Forgot to update the ChangeLog after the bug rename. (8.60 KB, patch)
2012-12-18 15:53 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Updated patch 4: fixed signed / unsigned comparison in the tests. (8.57 KB, patch)
2012-12-19 08:29 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Updated patch 5: sigh, fix the EWS. (8.56 KB, patch)
2012-12-19 10:00 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Proposed fix 6: This time passing on mac FOR REAL. (8.48 KB, patch)
2012-12-19 14:18 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2012-04-12 18:41:05 PDT
Add size_t versions of operator* and operator/
Comment 1 Emil A Eklund 2012-04-12 18:46:13 PDT
Created attachment 137018 [details]
Patch
Comment 2 Build Bot 2012-04-12 19:12:57 PDT
Comment on attachment 137018 [details]
Patch

Attachment 137018 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12395558
Comment 3 Early Warning System Bot 2012-04-12 19:19:36 PDT
Comment on attachment 137018 [details]
Patch

Attachment 137018 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12395561
Comment 4 Early Warning System Bot 2012-04-12 19:30:42 PDT
Comment on attachment 137018 [details]
Patch

Attachment 137018 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12395567
Comment 5 Emil A Eklund 2012-04-13 11:43:43 PDT
Created attachment 137117 [details]
Patch
Comment 6 Emil A Eklund 2012-04-13 13:11:09 PDT
Created attachment 137132 [details]
Patch
Comment 7 Emil A Eklund 2012-04-13 13:13:20 PDT
The only difference between patch2 and 3 is the ChangeLog entry.
Comment 8 Julien Chaffraix 2012-04-13 15:49:29 PDT
Comment on attachment 137132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137132&action=review

> Source/WebCore/platform/FractionalLayoutUnit.h:40
> +#define LAYOUT_UNIT_SIZE_T

Not a huge fan of the naming. First it should be an HAVE() macro as it is about the underlying platform. Then the name doesn't speak to me, how about HAVE_SEPARATE_SIZE_T_UNIT? (it really should be ARE_SIZE_T_UNSIGNED_DIFFERENT_UNIT but we don't have a general ARE() macro).

I wonder if we shouldn't put that in Platform.h as we do with the other macros, which would force rebuilding everything. Maybe someone else has some ideas on the matter.
Comment 9 Emil A Eklund 2012-04-13 16:17:37 PDT
(In reply to comment #8)
> (From update of attachment 137132 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137132&action=review
> 
> > Source/WebCore/platform/FractionalLayoutUnit.h:40
> > +#define LAYOUT_UNIT_SIZE_T
> 
> Not a huge fan of the naming. First it should be an HAVE() macro as it is about the underlying platform. Then the name doesn't speak to me, how about HAVE_SEPARATE_SIZE_T_UNIT? (it really should be ARE_SIZE_T_UNSIGNED_DIFFERENT_UNIT but we don't have a general ARE() macro).
> 
> I wonder if we shouldn't put that in Platform.h as we do with the other macros, which would force rebuilding everything. Maybe someone else has some ideas on the matter.

Seems fair. Do you think we should have a separate macro or just repeat the or clause for each function?
Comment 10 Julien Chaffraix 2012-04-13 16:48:10 PDT
Comment on attachment 137132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137132&action=review

>>> Source/WebCore/platform/FractionalLayoutUnit.h:40
>>> +#define LAYOUT_UNIT_SIZE_T
>> 
>> Not a huge fan of the naming. First it should be an HAVE() macro as it is about the underlying platform. Then the name doesn't speak to me, how about HAVE_SEPARATE_SIZE_T_UNIT? (it really should be ARE_SIZE_T_UNSIGNED_DIFFERENT_UNIT but we don't have a general ARE() macro).
>> 
>> I wonder if we shouldn't put that in Platform.h as we do with the other macros, which would force rebuilding everything. Maybe someone else has some ideas on the matter.
> 
> Seems fair. Do you think we should have a separate macro or just repeat the or clause for each function?

If nobody objects to an inline definition, I would rather use the ARE_SIZE_T_UNSIGNED_DIFFERENT_UNIT (or equivalent), repeated each time.
Comment 11 Emil A Eklund 2012-04-14 18:37:51 PDT
Created attachment 137222 [details]
Patch
Comment 12 Eric Seidel 2012-04-14 20:16:29 PDT
Comment on attachment 137222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137222&action=review

> Source/WebCore/platform/FractionalLayoutUnit.h:41
> +#if PLATFORM(CHROMIUM) || PLATFORM(MAC)
> +#define ARE_SIZE_T_UNSIGNED_DIFFERENT_UNIT
> +#endif

This feels like something which belongs in a more central place.  I'm not sure.  MathExtras?  Maybe there is some other header in wtf in which thsi would make sense?  Is this really the only place we have this trouble?
Comment 13 Emil A Eklund 2012-04-16 09:26:16 PDT
(In reply to comment #12)
> (From update of attachment 137222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137222&action=review
> 
> > Source/WebCore/platform/FractionalLayoutUnit.h:41
> > +#if PLATFORM(CHROMIUM) || PLATFORM(MAC)
> > +#define ARE_SIZE_T_UNSIGNED_DIFFERENT_UNIT
> > +#endif
> 
> This feels like something which belongs in a more central place.  I'm not sure.  MathExtras?  Maybe there is some other header in wtf in which thsi would make sense?  Is this really the only place we have this trouble?

I haven't found any other places where we have a function that can either take an unsigned int or a size_t, generally we tend to use one or the other.
Comment 14 Emil A Eklund 2012-04-17 10:07:14 PDT
I'm happy to move the definition elsewhere if you think that makes sense. Another option would be to keep it where it is for now and move it once we start using it elsewhere.
Comment 15 Julien Chaffraix 2012-04-17 10:24:51 PDT
Comment on attachment 137222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137222&action=review

>>> Source/WebCore/platform/FractionalLayoutUnit.h:41
>>> +#endif
>> 
>> This feels like something which belongs in a more central place.  I'm not sure.  MathExtras?  Maybe there is some other header in wtf in which thsi would make sense?  Is this really the only place we have this trouble?
> 
> I haven't found any other places where we have a function that can either take an unsigned int or a size_t, generally we tend to use one or the other.

I haven't heard any strong objections to keeping the #define here. The only people who commented felt like it may belong somewhere else but nothing substantial (Eric thought of MathExtras.h, I thought about Platform.h but it felt clunky). Based on your assessment, I feel like it's fine to keep it here for now. If someone objects, it can moved appropriately.
Comment 16 WebKit Review Bot 2012-04-17 11:52:08 PDT
Comment on attachment 137222 [details]
Patch

Clearing flags on attachment: 137222

Committed r114404: <http://trac.webkit.org/changeset/114404>
Comment 17 WebKit Review Bot 2012-04-17 11:52:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Emil A Eklund 2012-04-17 13:59:25 PDT
Reverted as it broke a couple of the chromium bots.

I'm not convinced this approach is going to work as the chromium linux release and valgrind bots seems to have conflicting ideas of whether or not size_t and unisgned are interchangeable, despite having the same architecture.
Comment 19 Darin Adler 2012-05-12 08:53:17 PDT
We should not add size_t versions, since our current overloading is based on basic C types. Instead we should add all the basic C types (except maybe the char ones), short, int, long, long long, unsigned short, unsigned, unsigned long, unsigned long long. Once we have all those covered, that takes care of size_t automatically because it’s one of those types.
Comment 20 Julien Chaffraix 2012-12-18 15:07:51 PST
Created attachment 180037 [details]
Updated patch: add overloaded operators for most unsigned types, per Darin's suggestion.
Comment 21 Levi Weintraub 2012-12-18 15:20:14 PST
Comment on attachment 180037 [details]
Updated patch: add overloaded operators for most unsigned types, per Darin's suggestion.

View in context: https://bugs.webkit.org/attachment.cgi?id=180037&action=review

> Tools/TestWebKitAPI/Tests/WebCore/LayoutUnit.cpp:194
> +    ASSERT_EQ((LayoutUnit(400) / aHundredSizeT).toInt(), aHundredSizeT);

This appears to say 400/100 = 100?
Comment 22 Emil A Eklund 2012-12-18 15:20:43 PST
Comment on attachment 180037 [details]
Updated patch: add overloaded operators for most unsigned types, per Darin's suggestion.

View in context: https://bugs.webkit.org/attachment.cgi?id=180037&action=review

> Source/WebCore/ChangeLog:3
> +        Add size_t versions of multiplication and division operators to FractionalLayoutUnit

You should update the bug description as you are in fact not adding size_t versions of the operators.

> Tools/TestWebKitAPI/Tests/WebCore/LayoutUnit.cpp:150
> +    size_t aHundredSizeT = 100;

I'd like to see a test for cases where the value is larger than MAX_INT.

Something like the quarterMax code below but with std::numeric_limits<size_t>::max.
Comment 23 Julien Chaffraix 2012-12-18 15:50:40 PST
Created attachment 180045 [details]
Updated patch without the bad multipliciation test and with an overflowing test.
Comment 24 Julien Chaffraix 2012-12-18 15:53:40 PST
Created attachment 180046 [details]
Updated patch 3: Forgot to update the ChangeLog after the bug rename.
Comment 25 Build Bot 2012-12-18 19:32:26 PST
Comment on attachment 180046 [details]
Updated patch 3: Forgot to update the ChangeLog after the bug rename.

Attachment 180046 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15404492
Comment 26 Julien Chaffraix 2012-12-19 08:29:45 PST
Created attachment 180172 [details]
Updated patch 4: fixed signed / unsigned comparison in the tests.
Comment 27 Build Bot 2012-12-19 09:09:39 PST
Comment on attachment 180172 [details]
Updated patch 4: fixed signed / unsigned comparison in the tests.

Attachment 180172 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15401700
Comment 28 WebKit Review Bot 2012-12-19 09:26:03 PST
Comment on attachment 180172 [details]
Updated patch 4: fixed signed / unsigned comparison in the tests.

Attachment 180172 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15410707
Comment 29 EFL EWS Bot 2012-12-19 09:28:28 PST
Comment on attachment 180172 [details]
Updated patch 4: fixed signed / unsigned comparison in the tests.

Attachment 180172 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15413652
Comment 30 Peter Beverloo (cr-android ews) 2012-12-19 09:57:08 PST
Comment on attachment 180172 [details]
Updated patch 4: fixed signed / unsigned comparison in the tests.

Attachment 180172 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15405716
Comment 31 Julien Chaffraix 2012-12-19 10:00:33 PST
Created attachment 180187 [details]
Updated patch 5: sigh, fix the EWS.
Comment 32 Build Bot 2012-12-19 13:04:34 PST
Comment on attachment 180187 [details]
Updated patch 5: sigh, fix the EWS.

Attachment 180187 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15401746
Comment 33 Build Bot 2012-12-19 14:08:56 PST
Comment on attachment 180187 [details]
Updated patch 5: sigh, fix the EWS.

Attachment 180187 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15413711
Comment 34 Julien Chaffraix 2012-12-19 14:18:05 PST
Created attachment 180225 [details]
Proposed fix 6: This time passing on mac FOR REAL.
Comment 35 Levi Weintraub 2012-12-20 09:47:30 PST
(In reply to comment #34)
> Created an attachment (id=180225) [details]
> Proposed fix 6: This time passing on mac FOR REAL.

Woo!
Comment 36 WebKit Review Bot 2013-01-07 09:26:58 PST
Comment on attachment 180225 [details]
Proposed fix 6: This time passing on mac FOR REAL.

Clearing flags on attachment: 180225

Committed r138952: <http://trac.webkit.org/changeset/138952>
Comment 37 WebKit Review Bot 2013-01-07 09:27:04 PST
All reviewed patches have been landed.  Closing bug.