Closed Bug 666984 Opened 13 years ago Closed 13 years ago

Remove/substitute NS_FloatIsFinite since it's the same as NS_finite


(Core :: DOM: Core & HTML, defect)

Not set





(Reporter: Ms2ger, Assigned: emorley)



(1 file, 3 obsolete files)

One of these is more than enough... CC'ing somebody who might be interested :)
Assignee: nobody → bmo
Attached patch Patch v1 (obsolete) — Splinter Review

First pass at this, let me know if this wasn't the intended approach.

Compiles fine locally, with no warnings on the changed lines (the rest of /content/svg/ is another matter, but don't get me started! More to do in bug 187528, but anyway).

However, slight query about how liberal I should be about #includes ? ie: I've added a few to make it compile, but should I be including nsMathUtils.h in absolutely every file I've added NS_finite into, to prevent it only being included by chance via another file? 

Likewise, in the files where I've s/NS_FloatIsFinite/NS_finite/g, I've removed the existing #include "nsContentUtils.h", on the premise that NS_FloatIsFinite is no longer being used. However it's plausible that nsContentUtils.h is being used for other stuff and it only compiles by chance inclusion elsewhere. 

Basically I don't want to make the bug 557565 / bug 634839 type situation any worse - and don't know what the recommendations are here?
Attachment #544512 - Flags: feedback?(Ms2ger)
The "no bootlegging" principle would suggest including nsMathUtils.h everywhere. For nsContentUtils.h, I'd remove it (unless the file uses an nsContentUtils::* function, of course).
Comment on attachment 544512 [details] [diff] [review]
Patch v1

>--- a/content/svg/content/src/SVGLength.h
>+++ b/content/svg/content/src/SVGLength.h

>-#include "nsContentUtils.h"

>--- a/content/svg/content/src/SVGPoint.h
>+++ b/content/svg/content/src/SVGPoint.h

>-#include "nsContentUtils.h"


>--- a/xpcom/ds/nsMathUtils.h
>+++ b/xpcom/ds/nsMathUtils.h


/**, please.

>+ * Check whether a floating point number is finite (not +/-infinity and not a
>+ * NaN value).
>+ */
> inline NS_HIDDEN_(bool) NS_finite(double d)
> {
> #ifdef WIN32
>     // NOTE: '!!' casts an int to bool without spamming MSVC warning C4800.
>     return !!_finite(d);
> #else
>     return finite(d);
> #endif

Looks great!
Attachment #544512 - Flags: feedback?(Ms2ger) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
Thanks Ms2ger :-)

Same as prior patch, other than nsMathUtils.h being included everywhere to avoid bootlegging + the nit for the nsMathUtils.h comment style.
Attachment #544512 - Attachment is obsolete: true
Attachment #544578 - Flags: review?(jwatt)
Summary: s/NS_FloatIsFinite/NS_finite/g → Remove/substitute NS_FloatIsFinite since it's the same as NS_finite
Flags: in-testsuite-
Ping for review. Thanks :-)
Attachment #544578 - Flags: review?(Olli.Pettay)
Comment on attachment 544578 [details] [diff] [review]
Patch v2

Sorry, totally forgot about this. r=jwatt although it would be nice if you could tidy up the trailing "\" chars on the lines in nsContentUtils.h.
Attachment #544578 - Flags: review?(jwatt)
Attachment #544578 - Flags: review?(Olli.Pettay)
Attachment #544578 - Flags: review+
Attached patch Patch v2.1 (obsolete) — Splinter Review
Updated to tip, tidied nsContentUtils.h trailing "\" chars; carrying forwards r+.

Thanks Jonathan :-)
Attachment #544578 - Attachment is obsolete: true
Attachment #549823 - Flags: review+
Attached patch Patch v2.2Splinter Review
Added missing #include "nsMathUtils.h" in nsSVGIntegerPair.cpp after updating to tip. Carrying forwards r+.
Attachment #549823 - Attachment is obsolete: true
Attachment #549830 - Flags: review+
Keywords: checkin-needed
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.