Bug 83848 - Support size_t multiplication and division operators on LayoutUnit
: Support size_t multiplication and division operators on LayoutUnit
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 60318 104700
  Show dependency treegraph
 
Reported: 2012-04-12 18:41 PST by
Modified: 2013-01-07 09:27 PST (History)


Attachments
Patch (3.90 KB, patch)
2012-04-12 18:46 PST, Emil A Eklund
no flags Review Patch | Details | Formatted Diff | Diff
Patch (3.88 KB, patch)
2012-04-13 11:43 PST, Emil A Eklund
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.00 KB, patch)
2012-04-13 13:11 PST, Emil A Eklund
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.11 KB, patch)
2012-04-14 18:37 PST, Emil A Eklund
no flags Review Patch | 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 Review Patch | 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 Review Patch | 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 Review Patch | 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 Review Patch | 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 Review Patch | 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 Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-12 18:41:05 PST
Add size_t versions of operator* and operator/
------- Comment #1 From 2012-04-12 18:46:13 PST -------
Created an attachment (id=137018) [details]
Patch
------- Comment #2 From 2012-04-12 19:12:57 PST -------
(From update of attachment 137018 [details])
Attachment 137018 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12395558
------- Comment #3 From 2012-04-12 19:19:36 PST -------
(From update of attachment 137018 [details])
Attachment 137018 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12395561
------- Comment #4 From 2012-04-12 19:30:42 PST -------
(From update of attachment 137018 [details])
Attachment 137018 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12395567
------- Comment #5 From 2012-04-13 11:43:43 PST -------
Created an attachment (id=137117) [details]
Patch
------- Comment #6 From 2012-04-13 13:11:09 PST -------
Created an attachment (id=137132) [details]
Patch
------- Comment #7 From 2012-04-13 13:13:20 PST -------
The only difference between patch2 and 3 is the ChangeLog entry.
------- Comment #8 From 2012-04-13 15:49:29 PST -------
(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.
------- Comment #9 From 2012-04-13 16:17:37 PST -------
(In reply to comment #8)
> (From update of attachment 137132 [details] [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 From 2012-04-13 16:48:10 PST -------
(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?

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 From 2012-04-14 18:37:51 PST -------
Created an attachment (id=137222) [details]
Patch
------- Comment #12 From 2012-04-14 20:16:29 PST -------
(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?
------- Comment #13 From 2012-04-16 09:26:16 PST -------
(In reply to comment #12)
> (From update of attachment 137222 [details] [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 From 2012-04-17 10:07:14 PST -------
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 From 2012-04-17 10:24:51 PST -------
(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
>>> +#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 From 2012-04-17 11:52:08 PST -------
(From update of attachment 137222 [details])
Clearing flags on attachment: 137222

Committed r114404: <http://trac.webkit.org/changeset/114404>
------- Comment #17 From 2012-04-17 11:52:14 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #18 From 2012-04-17 13:59:25 PST -------
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 From 2012-05-12 08:53:17 PST -------
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 From 2012-12-18 15:07:51 PST -------
Created an attachment (id=180037) [details]
Updated patch: add overloaded operators for most unsigned types, per Darin's suggestion.
------- Comment #21 From 2012-12-18 15:20:14 PST -------
(From update of attachment 180037 [details])
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 From 2012-12-18 15:20:43 PST -------
(From update of attachment 180037 [details])
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 From 2012-12-18 15:50:40 PST -------
Created an attachment (id=180045) [details]
Updated patch without the bad multipliciation test and with an overflowing test.
------- Comment #24 From 2012-12-18 15:53:40 PST -------
Created an attachment (id=180046) [details]
Updated patch 3: Forgot to update the ChangeLog after the bug rename.
------- Comment #25 From 2012-12-18 19:32:26 PST -------
(From update of attachment 180046 [details])
Attachment 180046 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15404492
------- Comment #26 From 2012-12-19 08:29:45 PST -------
Created an attachment (id=180172) [details]
Updated patch 4: fixed signed / unsigned comparison in the tests.
------- Comment #27 From 2012-12-19 09:09:39 PST -------
(From update of attachment 180172 [details])
Attachment 180172 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15401700
------- Comment #28 From 2012-12-19 09:26:03 PST -------
(From update of attachment 180172 [details])
Attachment 180172 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15410707
------- Comment #29 From 2012-12-19 09:28:28 PST -------
(From update of attachment 180172 [details])
Attachment 180172 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15413652
------- Comment #30 From 2012-12-19 09:57:08 PST -------
(From update of attachment 180172 [details])
Attachment 180172 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15405716
------- Comment #31 From 2012-12-19 10:00:33 PST -------
Created an attachment (id=180187) [details]
Updated patch 5: sigh, fix the EWS.
------- Comment #32 From 2012-12-19 13:04:34 PST -------
(From update of attachment 180187 [details])
Attachment 180187 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15401746
------- Comment #33 From 2012-12-19 14:08:56 PST -------
(From update of attachment 180187 [details])
Attachment 180187 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15413711
------- Comment #34 From 2012-12-19 14:18:05 PST -------
Created an attachment (id=180225) [details]
Proposed fix 6: This time passing on mac FOR REAL.
------- Comment #35 From 2012-12-20 09:47:30 PST -------
(In reply to comment #34)
> Created an attachment (id=180225) [details] [details]
> Proposed fix 6: This time passing on mac FOR REAL.

Woo!
------- Comment #36 From 2013-01-07 09:26:58 PST -------
(From update of attachment 180225 [details])
Clearing flags on attachment: 180225

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