Closed
Bug 781313
Opened 12 years ago
Closed 12 years ago
Move the Euclid gcd and lcm algos from layout/style/nsStyleAnimation.cpp to mfbt/MathAlgorithms.h
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: raphc, Assigned: raphc)
References
Details
Attachments
(1 file, 8 obsolete files)
4.76 KB,
patch
|
mounir
:
checkin+
|
Details | Diff | Splinter Review |
Move these algos to nsMathUtils.h to be able to reuse them.
Assignee | ||
Updated•12 years ago
|
Summary: Move the Eucllid gcd and lcm algos form layout/style/nsStyleAnimation.cpp to nsMathUtils.h → Move the Eucllid gcd and lcm algos from layout/style/nsStyleAnimation.cpp to nsMathUtils.h
Assignee | ||
Comment 1•12 years ago
|
||
Mounir, do you know who I could ask for a review?
Assignee: nobody → rcatolino
Status: NEW → ASSIGNED
Comment 2•12 years ago
|
||
Comment on attachment 650272 [details] [diff] [review] patch Maybe Doug can do this review quickly enough? :)
Attachment #650272 -
Flags: review?(doug.turner)
Assignee | ||
Comment 3•12 years ago
|
||
I changed the arguments from PRUint32 to PRUint64.
Attachment #650272 -
Attachment is obsolete: true
Attachment #650272 -
Flags: review?(doug.turner)
Attachment #650522 -
Flags: review?(doug.turner)
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #650522 -
Attachment is obsolete: true
Attachment #650522 -
Flags: review?(doug.turner)
Attachment #650523 -
Flags: review?(doug.turner)
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #650523 -
Attachment is obsolete: true
Attachment #650523 -
Flags: review?(doug.turner)
Attachment #650547 -
Flags: review?(doug.turner)
Comment 6•12 years ago
|
||
Comment on attachment 650547 [details] [diff] [review] patch not the right reviewer for layout changes.
Attachment #650547 -
Flags: review?(doug.turner) → review?(dbaron)
(In reply to Raphael Catolino (:raphc) from comment #3) > I changed the arguments from PRUint32 to PRUint64. Why? Won't that make it more expensive on many architectures?
Summary: Move the Eucllid gcd and lcm algos from layout/style/nsStyleAnimation.cpp to nsMathUtils.h → Move the Euclid gcd and lcm algos from layout/style/nsStyleAnimation.cpp to nsMathUtils.h
Assignee | ||
Comment 8•12 years ago
|
||
I need to use it with bigger numbers (I use it on timestamps) in bug 769359. If the memory cost is too high in nsStyleAnimation, I can do a second implementation for 32bit values. Maybe using a macro would be better then?
I think you should: * call them NS_EuclidLCM and NS_EuclidGCD * make them function templates, i.e., "template <typename IntegerType>" Otherwise this looks fine, but I'd like to look at the revisions.
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #650547 -
Attachment is obsolete: true
Attachment #650547 -
Flags: review?(dbaron)
Attachment #650863 -
Flags: review?(dbaron)
Comment 11•12 years ago
|
||
Might as well move them into MFBT
Comment 12•12 years ago
|
||
Do we have anything for maths there?
Comment on attachment 650863 [details] [diff] [review] patch Please capitalize all the letters of "LCM" and "GCD". They're abbreviations, so all letters should be capital. r=dbaron with that
Attachment #650863 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 14•12 years ago
|
||
I moved it to mfbt/Maths.h as it seemed a better place than xpcom.
Attachment #650863 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #651139 -
Flags: review+
Comment 15•12 years ago
|
||
Comment on attachment 651139 [details] [diff] [review] patch Review of attachment 651139 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, doesn't work. You can't include nsDebug.h in MFBT headers. ::: mfbt/Maths.h @@ +7,5 @@ > + > +#ifndef mozilla_Maths_h_ > +#define mozilla_Maths_h_ > + > +#include "nsDebug.h" Please include mozilla/Assertions.h @@ +16,5 @@ > +EuclidGCD(IntegerType a, IntegerType b) > +{ > + // Euclid's algorithm; O(N) in the worst case. (There are better > + // ways, but we don't need them for the current use of this algo.) > + NS_ABORT_IF_FALSE(a > 0 && b > 0, "positive numbers expected"); And use MOZ_ASSERT(a > 0); MOZ_ASSERT(b > 0);
Attachment #651139 -
Flags: review+ → review-
If you're going to proceed with moving it into mfbt, please call it Math.h rather than Maths.h. And if it's in mfbt it should be in namespace mozilla as well. Additionally, I think you'd need review from whoever owns mfbt. (I don't know who that is.)
Assignee | ||
Comment 17•12 years ago
|
||
Ms2ger, do you have any idea who can review this?
Attachment #651139 -
Attachment is obsolete: true
Assignee | ||
Comment 18•12 years ago
|
||
I moved it back to Maths.h not to break the build on case insensitive file systems as described in https://bugzilla.mozilla.org/show_bug.cgi?id=776429#c3.
Attachment #651285 -
Attachment is obsolete: true
Attachment #651345 -
Flags: review?(Ms2ger)
Comment 19•12 years ago
|
||
Comment on attachment 651345 [details] [diff] [review] patch Given the earlier r+ from dbaron, I'll rubber-stamp as an mfbt peer with one request: can the name be changed to something less general than Maths.h? MathAlgorithms.h seems nice and specific, but I'd be happy to hear other suggestions.
Attachment #651345 -
Flags: review?(Ms2ger) → review+
Comment 20•12 years ago
|
||
Raphael, can you attach an updated patch with Luke's suggestions?
Assignee | ||
Comment 21•12 years ago
|
||
Yep sorry for the delay, I've pushed my patch queue to try, with this patch in it, and I wanted to wait for the result before posting it, but it looks like it will take forever, so here it is. Mounir can you do the chekin?
Attachment #651345 -
Attachment is obsolete: true
Attachment #652470 -
Flags: checkin?(mounir)
Updated•12 years ago
|
Flags: in-testsuite-
Target Milestone: --- → mozilla17
Updated•12 years ago
|
Attachment #652470 -
Flags: checkin?(mounir) → checkin+
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b2ad6e5fc690
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Component: XPCOM → MFBT
Summary: Move the Euclid gcd and lcm algos from layout/style/nsStyleAnimation.cpp to nsMathUtils.h → Move the Euclid gcd and lcm algos from layout/style/nsStyleAnimation.cpp to mfbt/MathAlgorithms.h
You need to log in
before you can comment on or make changes to this bug.
Description
•