Last Comment Bug 781313 - Move the Euclid gcd and lcm algos from layout/style/nsStyleAnimation.cpp to mfbt/MathAlgorithms.h
: Move the Euclid gcd and lcm algos from layout/style/nsStyleAnimation.cpp to m...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Raphael Catolino (:raphc)
:
Mentors:
Depends on:
Blocks: 769359
  Show dependency treegraph
 
Reported: 2012-08-08 12:32 PDT by Raphael Catolino (:raphc)
Modified: 2012-10-04 18:06 PDT (History)
6 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.40 KB, patch)
2012-08-08 13:10 PDT, Raphael Catolino (:raphc)
no flags Details | Diff | Splinter Review
patch (4.49 KB, patch)
2012-08-09 06:07 PDT, Raphael Catolino (:raphc)
no flags Details | Diff | Splinter Review
patch (4.34 KB, patch)
2012-08-09 06:16 PDT, Raphael Catolino (:raphc)
no flags Details | Diff | Splinter Review
patch (4.34 KB, patch)
2012-08-09 07:25 PDT, Raphael Catolino (:raphc)
no flags Details | Diff | Splinter Review
patch (4.45 KB, patch)
2012-08-10 06:33 PDT, Raphael Catolino (:raphc)
dbaron: review+
Details | Diff | Splinter Review
patch (4.65 KB, patch)
2012-08-11 11:25 PDT, Raphael Catolino (:raphc)
Ms2ger: review-
Details | Diff | Splinter Review
patch (4.67 KB, patch)
2012-08-13 01:45 PDT, Raphael Catolino (:raphc)
no flags Details | Diff | Splinter Review
patch (4.68 KB, patch)
2012-08-13 06:15 PDT, Raphael Catolino (:raphc)
luke: review+
Details | Diff | Splinter Review
patch (4.76 KB, patch)
2012-08-16 09:04 PDT, Raphael Catolino (:raphc)
mounir: checkin+
Details | Diff | Splinter Review

Description Raphael Catolino (:raphc) 2012-08-08 12:32:22 PDT
Move these algos to nsMathUtils.h to be able to reuse them.
Comment 1 Raphael Catolino (:raphc) 2012-08-08 13:10:09 PDT
Created attachment 650272 [details] [diff] [review]
patch

Mounir, do you know who I could ask for a review?
Comment 2 Mounir Lamouri (:mounir) 2012-08-09 05:59:46 PDT
Comment on attachment 650272 [details] [diff] [review]
patch

Maybe Doug can do this review quickly enough? :)
Comment 3 Raphael Catolino (:raphc) 2012-08-09 06:07:08 PDT
Created attachment 650522 [details] [diff] [review]
patch

I changed the arguments from PRUint32 to PRUint64.
Comment 4 Raphael Catolino (:raphc) 2012-08-09 06:16:59 PDT
Created attachment 650523 [details] [diff] [review]
patch
Comment 5 Raphael Catolino (:raphc) 2012-08-09 07:25:53 PDT
Created attachment 650547 [details] [diff] [review]
patch
Comment 6 Doug Turner (:dougt) 2012-08-09 09:22:00 PDT
Comment on attachment 650547 [details] [diff] [review]
patch

not the right reviewer for layout changes.
Comment 7 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2012-08-09 09:44:24 PDT
(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?
Comment 8 Raphael Catolino (:raphc) 2012-08-09 09:53:31 PDT
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?
Comment 9 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2012-08-09 11:32:54 PDT
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.
Comment 10 Raphael Catolino (:raphc) 2012-08-10 06:33:08 PDT
Created attachment 650863 [details] [diff] [review]
patch
Comment 11 :Ms2ger 2012-08-10 06:37:08 PDT
Might as well move them into MFBT
Comment 12 Mounir Lamouri (:mounir) 2012-08-10 06:44:59 PDT
Do we have anything for maths there?
Comment 13 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2012-08-10 14:03:06 PDT
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
Comment 14 Raphael Catolino (:raphc) 2012-08-11 11:25:28 PDT
Created attachment 651139 [details] [diff] [review]
patch

I moved it to mfbt/Maths.h as it seemed a better place than xpcom.
Comment 15 :Ms2ger 2012-08-11 12:42:19 PDT
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);
Comment 16 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2012-08-11 13:11:11 PDT
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.)
Comment 17 Raphael Catolino (:raphc) 2012-08-13 01:45:11 PDT
Created attachment 651285 [details] [diff] [review]
patch

Ms2ger, do you have any idea who can review this?
Comment 18 Raphael Catolino (:raphc) 2012-08-13 06:15:51 PDT
Created attachment 651345 [details] [diff] [review]
patch

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.
Comment 19 Luke Wagner [:luke] 2012-08-14 12:16:45 PDT
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.
Comment 20 Mounir Lamouri (:mounir) 2012-08-16 08:47:38 PDT
Raphael, can you attach an updated patch with Luke's suggestions?
Comment 21 Raphael Catolino (:raphc) 2012-08-16 09:04:56 PDT
Created attachment 652470 [details] [diff] [review]
patch

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?

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