Last Comment Bug 666984 - Remove/substitute NS_FloatIsFinite since it's the same as NS_finite
: Remove/substitute NS_FloatIsFinite since it's the same as NS_finite
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Ed Morley [:emorley]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-24 11:09 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2011-08-02 03:20 PDT (History)
4 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (22.63 KB, patch)
2011-07-07 09:09 PDT, Ed Morley [:emorley]
Ms2ger: feedback+
Details | Diff | Splinter Review
Patch v2 (28.70 KB, patch)
2011-07-07 12:14 PDT, Ed Morley [:emorley]
jwatt: review+
Details | Diff | Splinter Review
Patch v2.1 (29.43 KB, patch)
2011-08-01 09:23 PDT, Ed Morley [:emorley]
emorley: review+
Details | Diff | Splinter Review
Patch v2.2 (29.90 KB, patch)
2011-08-01 09:28 PDT, Ed Morley [:emorley]
emorley: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2011-06-24 11:09:39 PDT
One of these is more than enough... CC'ing somebody who might be interested :)
Comment 1 Ed Morley [:emorley] 2011-07-07 09:09:02 PDT
Created attachment 544512 [details] [diff] [review]
Patch v1

http://mxr.mozilla.org/mozilla-central/search?string=NS_FloatIsFinite
http://mxr.mozilla.org/mozilla-central/search?string=NS_finite

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?
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2011-07-07 10:59:21 PDT
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 3 :Ms2ger (⌚ UTC+1/+2) 2011-07-07 11:02:48 PDT
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"

Excellent.

>--- 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!
Comment 4 Ed Morley [:emorley] 2011-07-07 12:14:30 PDT
Created attachment 544578 [details] [diff] [review]
Patch v2

Thanks Ms2ger :-)

Same as prior patch, other than nsMathUtils.h being included everywhere to avoid bootlegging + the nit for the nsMathUtils.h comment style.
Comment 5 Ed Morley [:emorley] 2011-07-09 04:11:42 PDT
http://dev.philringnalda.com/tbpl/?tree=Try&rev=7e2dec83b318
Comment 6 Ed Morley [:emorley] 2011-07-25 05:50:53 PDT
Ping for review. Thanks :-)
Comment 7 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2011-08-01 08:08:00 PDT
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.
Comment 8 Ed Morley [:emorley] 2011-08-01 09:23:05 PDT
Created attachment 549823 [details] [diff] [review]
Patch v2.1

Updated to tip, tidied nsContentUtils.h trailing "\" chars; carrying forwards r+.

Thanks Jonathan :-)
Comment 9 Ed Morley [:emorley] 2011-08-01 09:28:17 PDT
Created attachment 549830 [details] [diff] [review]
Patch v2.2

Added missing #include "nsMathUtils.h" in nsSVGIntegerPair.cpp after updating to tip. Carrying forwards r+.
Comment 11 Marco Bonardo [::mak] 2011-08-02 03:20:15 PDT
http://hg.mozilla.org/mozilla-central/rev/c5e091d2dd38

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