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

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Raphael Catolino (:raphc)]

Move these algos to nsMathUtils.h to be able to reuse them.

Comment 1

[Raphael Catolino (:raphc)]

Attachment: patch

Mounir, do you know who I could ask for a review?

Comment 2

[Mounir Lamouri (:mounir)]

Comment on attachment 650272 [details] [diff] [review]
patch

Maybe Doug can do this review quickly enough? :)

Comment 3

[Raphael Catolino (:raphc)]

Attachment: patch

I changed the arguments from PRUint32 to PRUint64.

Comment 4

[Raphael Catolino (:raphc)]

Attachment: patch

Comment 5

[Raphael Catolino (:raphc)]

Attachment: patch

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 650547 [details] [diff] [review]
patch

not the right reviewer for layout changes.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(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)]

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-5, no longer working on Mozilla)]

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)]

Attachment: patch

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Might as well move them into MFBT

Comment 12

[Mounir Lamouri (:mounir)]

Do we have anything for maths there?

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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)]

Attachment: patch

I moved it to mfbt/Maths.h as it seemed a better place than xpcom.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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-5, no longer working on Mozilla)]

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)]

Attachment: patch

Ms2ger, do you have any idea who can review this?

Comment 18

[Raphael Catolino (:raphc)]

Attachment: 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]]

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)]

Raphael, can you attach an updated patch with Luke's suggestions?

Comment 21

[Raphael Catolino (:raphc)]

Attachment: 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?

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/b2ad6e5fc690