Bug 83848 – Support size_t multiplication and division operators on LayoutUnit

• Component: Platform
• Status: RESOLVED FIXED

[Emil A Eklund]

Add size_t versions of operator* and operator/

[Emil A Eklund]

Created attachment 137018 [details]
Patch

[Build Bot]

Comment on attachment 137018 [details]
Patch

Attachment 137018 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12395558

[Early Warning System Bot]

Comment on attachment 137018 [details]
Patch

Attachment 137018 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12395561

[Early Warning System Bot]

Comment on attachment 137018 [details]
Patch

Attachment 137018 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12395567

[Emil A Eklund]

Created attachment 137117 [details]
Patch

[Emil A Eklund]

Created attachment 137132 [details]
Patch

[Emil A Eklund]

The only difference between patch2 and 3 is the ChangeLog entry.

[Julien Chaffraix]

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.

[Emil A Eklund]

(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?

[Julien Chaffraix]

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.

[Emil A Eklund]

Created attachment 137222 [details]
Patch

[Eric Seidel (no email)]

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?

[Emil A Eklund]

(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.

[Emil A Eklund]

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.

[Julien Chaffraix]

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.

[WebKit Review Bot]

Comment on attachment 137222 [details]
Patch

Clearing flags on attachment: 137222

Committed r114404: <http://trac.webkit.org/changeset/114404>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Emil A Eklund]

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.

[Darin Adler]

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.

[Julien Chaffraix]

Created attachment 180037 [details]
Updated patch: add overloaded operators for most unsigned types, per Darin's suggestion.

[Levi Weintraub]

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?

[Emil A Eklund]

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.

[Julien Chaffraix]

Created attachment 180045 [details]
Updated patch without the bad multipliciation test and with an overflowing test.

[Julien Chaffraix]

Created attachment 180046 [details]
Updated patch 3: Forgot to update the ChangeLog after the bug rename.

[Build Bot]

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

[Julien Chaffraix]

Created attachment 180172 [details]
Updated patch 4: fixed signed / unsigned comparison in the tests.

[Build Bot]

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

[WebKit Review Bot]

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

[EFL EWS Bot]

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

[Peter Beverloo (cr-android ews)]

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

[Julien Chaffraix]

Created attachment 180187 [details]
Updated patch 5: sigh, fix the EWS.

[Build Bot]

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

[Build Bot]

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

[Julien Chaffraix]

Created attachment 180225 [details]
Proposed fix 6: This time passing on mac FOR REAL.

[Levi Weintraub]

(In reply to comment #34)
> Created an attachment (id=180225) [details]
> Proposed fix 6: This time passing on mac FOR REAL.

Woo!

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.