Add size_t versions of operator* and operator/
Created attachment 137018 [details] Patch
Comment on attachment 137018 [details] Patch Attachment 137018 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12395558
Comment on attachment 137018 [details] Patch Attachment 137018 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12395561
Comment on attachment 137018 [details] Patch Attachment 137018 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12395567
Created attachment 137117 [details] Patch
Created attachment 137132 [details] Patch
The only difference between patch2 and 3 is the ChangeLog entry.
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.
(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 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.
Created attachment 137222 [details] Patch
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?
(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.
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 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 on attachment 137222 [details] Patch Clearing flags on attachment: 137222 Committed r114404: <http://trac.webkit.org/changeset/114404>
All reviewed patches have been landed. Closing bug.
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.
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.
Created attachment 180037 [details] Updated patch: add overloaded operators for most unsigned types, per Darin's suggestion.
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 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.
Created attachment 180045 [details] Updated patch without the bad multipliciation test and with an overflowing test.
Created attachment 180046 [details] Updated patch 3: Forgot to update the ChangeLog after the bug rename.
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
Created attachment 180172 [details] Updated patch 4: fixed signed / unsigned comparison in the tests.
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 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 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 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
Created attachment 180187 [details] Updated patch 5: sigh, fix the EWS.
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 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
Created attachment 180225 [details] Proposed fix 6: This time passing on mac FOR REAL.
(In reply to comment #34) > Created an attachment (id=180225) [details] > Proposed fix 6: This time passing on mac FOR REAL. Woo!
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>