Closed Bug 781313 Opened 8 years ago Closed 8 years ago

Move the Euclid gcd and lcm algos from layout/style/nsStyleAnimation.cpp to mfbt/MathAlgorithms.h

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: raphc, Assigned: raphc)

References

Details

Attachments

(1 file, 8 obsolete files)

Move these algos to nsMathUtils.h to be able to reuse them.
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
Attached patch patch (obsolete) — Splinter Review
Mounir, do you know who I could ask for a review?
Assignee: nobody → rcatolino
Status: NEW → ASSIGNED
Blocks: 769359
Comment on attachment 650272 [details] [diff] [review]
patch

Maybe Doug can do this review quickly enough? :)
Attachment #650272 - Flags: review?(doug.turner)
Attached patch patch (obsolete) — Splinter Review
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)
Attached patch patch (obsolete) — Splinter Review
Attachment #650522 - Attachment is obsolete: true
Attachment #650522 - Flags: review?(doug.turner)
Attachment #650523 - Flags: review?(doug.turner)
Attached patch patch (obsolete) — Splinter Review
Attachment #650523 - Attachment is obsolete: true
Attachment #650523 - Flags: review?(doug.turner)
Attachment #650547 - Flags: review?(doug.turner)
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
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.
Attached patch patch (obsolete) — Splinter Review
Attachment #650547 - Attachment is obsolete: true
Attachment #650547 - Flags: review?(dbaron)
Attachment #650863 - Flags: review?(dbaron)
Might as well move them into MFBT
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+
Attached patch patch (obsolete) — Splinter Review
I moved it to mfbt/Maths.h as it seemed a better place than xpcom.
Attachment #650863 - Attachment is obsolete: true
Attachment #651139 - Flags: review+
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.)
Attached patch patch (obsolete) — Splinter Review
Ms2ger, do you have any idea who can review this?
Attachment #651139 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
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 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+
Raphael, can you attach an updated patch with Luke's suggestions?
Attached patch patchSplinter Review
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)
Flags: in-testsuite-
Target Milestone: --- → mozilla17
Attachment #652470 - Flags: checkin?(mounir) → checkin+
https://hg.mozilla.org/mozilla-central/rev/b2ad6e5fc690
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.