The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

Core
MFBT
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: raphc, Assigned: raphc)

Tracking

Trunk
mozilla17
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

5 years ago
Move these algos to nsMathUtils.h to be able to reuse them.
(Assignee)

Updated

5 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

5 years ago
Created attachment 650272 [details] [diff] [review]
patch

Mounir, do you know who I could ask for a review?
Assignee: nobody → rcatolino
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Blocks: 769359
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

5 years ago
Created attachment 650522 [details] [diff] [review]
patch

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

5 years ago
Created attachment 650523 [details] [diff] [review]
patch
Attachment #650522 - Attachment is obsolete: true
Attachment #650522 - Flags: review?(doug.turner)
Attachment #650523 - Flags: review?(doug.turner)
(Assignee)

Comment 5

5 years ago
Created attachment 650547 [details] [diff] [review]
patch
Attachment #650523 - Attachment is obsolete: true
Attachment #650523 - Flags: review?(doug.turner)
Attachment #650547 - Flags: review?(doug.turner)

Comment 6

5 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

5 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

5 years ago
Created attachment 650863 [details] [diff] [review]
patch
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+
(Assignee)

Comment 14

5 years ago
Created attachment 651139 [details] [diff] [review]
patch

I moved it to mfbt/Maths.h as it seemed a better place than xpcom.
Attachment #650863 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
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.)
(Assignee)

Comment 17

5 years ago
Created attachment 651285 [details] [diff] [review]
patch

Ms2ger, do you have any idea who can review this?
Attachment #651139 - Attachment is obsolete: true
(Assignee)

Comment 18

5 years ago
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.
Attachment #651285 - Attachment is obsolete: true
Attachment #651345 - Flags: review?(Ms2ger)

Comment 19

5 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+
Raphael, can you attach an updated patch with Luke's suggestions?
(Assignee)

Comment 21

5 years ago
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?
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
Last Resolved: 5 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.