Last Comment Bug 661584 - Code cleanup, substitute PR_(MAX|MIN|ABS|ROUNDUP) macro calls
: Code cleanup, substitute PR_(MAX|MIN|ABS|ROUNDUP) macro calls
Status: RESOLVED FIXED
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 7
Assigned To: Ed Morley [:emorley]
:
Mentors:
Depends on: 645398
Blocks: 518502
  Show dependency treegraph
 
Reported: 2011-06-02 10:55 PDT by :Ms2ger
Modified: 2011-07-06 15:35 PDT (History)
11 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
The simple s/PR_foo/NS_foo cases (45.91 KB, patch)
2011-06-15 10:07 PDT, Ed Morley [:emorley]
roc: review+
Details | Diff | Review
ssltunnel changes (4.88 KB, patch)
2011-06-15 10:20 PDT, Ed Morley [:emorley]
ted: review+
Details | Diff | Review
Everything else (24.16 KB, patch)
2011-06-15 10:24 PDT, Ed Morley [:emorley]
roc: review+
Details | Diff | Review
All patches qfolded for checkin (72.61 KB, patch)
2011-06-25 06:13 PDT, Ed Morley [:emorley]
emorley: review+
Details | Diff | Review
Loose ends follow-up (3.45 KB, patch)
2011-06-30 06:50 PDT, Ed Morley [:emorley]
roc: review+
Details | Diff | Review

Description :Ms2ger 2011-06-02 10:55:18 PDT
+++ This bug was initially created as a clone of Bug #645398 +++

We would like to remove the remaining PR_(MAX|MIN|ABS|ROUNDUP) macro calls and replace them with with NS_* inline calls.
Comment 1 Ed Morley [:emorley] 2011-06-15 10:07:46 PDT
Created attachment 539576 [details] [diff] [review]
The simple s/PR_foo/NS_foo cases

To make review easier, I've split this up into the more straight forwards s/PR_foo/NS_foo and then the ones that required casting or other changes.

The three patches folded together passed try: http://dev.philringnalda.com/tbpl/?tree=Try&rev=63acc1c93110
Comment 2 Ed Morley [:emorley] 2011-06-15 10:20:32 PDT
Created attachment 539579 [details] [diff] [review]
ssltunnel changes

The inclusion of nsAlgorithm.h caused build errors along the lines of:
ssltunnel.cpp(235) : warning C4003: not enough actual parameters for macro 'free'
...due to the local function free(), so I had to rename it, don't know if areafree() is the best choice, let me know if you'd prefer something else.

I had to add $(MOZALLOC_LIB) to Makefile LIBS, due to these build errors:
error LNK2019: unresolved external symbol __imp__moz_xmalloc ...
error LNK2019: unresolved external symbol __imp__moz_free ...
...again let me know if that wasn't the best way to do it.

Has passed try, see comment 1 for TBPL URL.
Comment 3 Ed Morley [:emorley] 2011-06-15 10:24:48 PDT
Created attachment 539580 [details] [diff] [review]
Everything else

Instances where I had to cast / it wasn't a straight substitution, for closer inspection. Think all the casts should be ok, but would feel happier if you had a quick sanity check. Thanks! :-)

Has passed try, URL in comment 1.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-15 16:41:45 PDT
Comment on attachment 539576 [details] [diff] [review]
The simple s/PR_foo/NS_foo cases

Review of attachment 539576 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-15 16:58:06 PDT
Comment on attachment 539580 [details] [diff] [review]
Everything else

Review of attachment 539580 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with those fixed

::: content/media/nsMediaCache.cpp
@@ +741,5 @@
>    // Cache size is in KB
>    PRInt32 cacheSize = Preferences::GetInt("media.cache_size", 500*1024);
>    PRInt64 maxBlocks = static_cast<PRInt64>(cacheSize)*1024/nsMediaCache::BLOCK_SIZE;
> +  maxBlocks = NS_MAX<PRInt64>(maxBlocks, 1);
> +  return NS_MIN<PRInt32>(maxBlocks, PR_INT32_MAX);

This isn't right. Casting maxBlocks down to PRInt32 could overflow. We need to do NS_MIN<PRInt64>, only then is it safe to cast the result to a PRInt32.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1086,5 @@
>          // NOT persistent, we simply accept everything in |buf|.
>          if (mConnection->IsPersistent() || mPreserveStream) {
>              PRInt64 remaining = mContentLength - mContentRead;
>              PRInt64 count64 = count;
> +            *contentRead = NS_MIN<PRUint32>(count64, remaining);

Similar to above, the MIN needs to happen in PRInt64 space. Also count64 should be removed, we can just use count here.

::: widget/src/windows/nsIMM32Handler.cpp
@@ +1211,5 @@
>      }
>    }
>    // compClauseArrayLength may be negative. I.e., ImmGetCompositionStringW
>    // may return an error code.
> +  mClauseArray.SetLength(NS_MAX<PRInt32>(0, clauseArrayLength));

Should just be <long>

@@ +1237,5 @@
>    }
>  
>    // attrStrLen may be negative. I.e., ImmGetCompositionStringW may return an
>    // error code.
> +  mAttributeArray.SetLength(NS_MAX<PRInt32>(0, attrArrayLength));

Should just be <long>
Comment 6 Ed Morley [:emorley] 2011-06-16 08:23:40 PDT
Have updated the patch locally with the comment 5 changes, but before I upload, I'd like to deal with some build warnings that I spotted after double checking the local build log. (Not sure if they existed before this patch, but they are on the lines I'm changing, so might as well fix them whilst I'm here).

/content/base/src/nsTextFragment.cpp(168) : warning C4146: unary minus operator applied to unsigned type, result still unsigned
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsTextFragment.cpp#168
> -  PR_MIN(len, PRInt32(((-NS_PTR_TO_UINT32(str)) & alignMask) / sizeof(PRUnichar)));
> +  NS_MIN(len, PRInt32(((-NS_PTR_TO_UINT32(str)) & alignMask) / sizeof(PRUnichar)));

/content/base/src/nsTextFragmentSSE2.cpp(39) : warning C4146: unary minus operator applied to unsigned type, result still unsigned
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsTextFragmentSSE2.cpp#38
> -  PR_MIN(len, PRInt32(((-NS_PTR_TO_UINT32(str)) & 0xf) / sizeof(PRUnichar)));
> +  NS_MIN(len, PRInt32(((-NS_PTR_TO_UINT32(str)) & 0xf) / sizeof(PRUnichar)));

/xpcom/string/src/nsUTF8UtilsSSE2.cpp(15) : warning C4146: unary minus operator applied to unsigned type, result still unsigned
http://mxr.mozilla.org/mozilla-central/source/xpcom/string/src/nsUTF8UtilsSSE2.cpp#14
> -  PR_MIN(aSourceLength, (-NS_PTR_TO_UINT32(aSource) & 0xf) / sizeof(PRUnichar));
> +  NS_MIN<PRUint32>(aSourceLength, (-NS_PTR_TO_UINT32(aSource) & 0xf) / sizeof(PRUnichar));

/xpcom/string/src/nsUTF8UtilsSSE2.cpp(68) : warning C4146: unary minus operator applied to unsigned type, result still unsigned
http://mxr.mozilla.org/mozilla-central/source/xpcom/string/src/nsUTF8UtilsSSE2.cpp#67
> -  PRUint32 alignLen = PR_MIN(aSourceLength, (-NS_PTR_TO_UINT32(aSource) & 0xf));
> +  PRUint32 alignLen = NS_MIN<PRUint32>(aSourceLength, (-NS_PTR_TO_UINT32(aSource) & 0xf));

What's the best way of dealing with these? Using NS_PTR_TO_INT32 instead and then casting back to PRUint32 if needed, after the NS_MIN? Or just dropping the minus operator entirely? (Unfortunately my C++ / NS_* knowledge is minimal at this point, so excuse the no doubt stupid question!)
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-16 21:08:54 PDT
PRUint32(-NS_PTR_TO_INT32(aSource) & 0xF)
Comment 9 Ted Mielczarek [:ted.mielczarek] 2011-06-24 09:50:12 PDT
Comment on attachment 539579 [details] [diff] [review]
ssltunnel changes

Review of attachment 539579 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/ssltunnel/Makefile.in
@@ +62,1 @@
>  	$(NULL)

Can you just reindent this block with two-space indent while you're here?
Comment 10 Ed Morley [:emorley] 2011-06-25 06:13:35 PDT
Created attachment 541926 [details] [diff] [review]
All patches qfolded for checkin

The three r+ patches qfolded for checkin, with the following changes:
- Comment 5 review nits.
- Comment 8 changes to silence build warnings mentioned in comment 6.
- Comment 9 ssltunnel indentation fix.
Comment 12 Mounir Lamouri (:mounir) 2011-06-27 02:10:00 PDT
Merged:
http://hg.mozilla.org/mozilla-central/rev/fc776fa4afb7
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-06-28 14:45:25 PDT
Comment on attachment 541926 [details] [diff] [review]
All patches qfolded for checkin

>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>--- a/layout/base/nsPresShell.cpp
>+++ b/layout/base/nsPresShell.cpp
>@@ -51,16 +51,17 @@
>  * identified per MPL Section 3.3
>  *
>  * Date         Modified by     Description of modification
>  * 05/03/2000   IBM Corp.       Observer events for reflow states
>  */
> 
> /* a presentation of a document, part 2 */
> 
>+#include "nsAlgorithm.h"
> #include "nsIPresShell.h"
> #include "nsPresContext.h"
> #include "nsIContent.h"
> #include "mozilla/dom/Element.h"
> #include "nsIDocument.h"
> #include "nsIDOMXULDocument.h"
> #include "nsStubDocumentObserver.h"
> #include "nsStyleSet.h"

I consider putting this include first to be bad practice.  In order to ensure that each header file, included on its own, compiles, it's good practice to put the "most related" header file first.  In other words, the first include in nsPresShell.cpp should be nsIPresShell.h so that there's some file (nsPresShell.cpp) where the first include is nsIPresShell.h, so that if somebody adds something to nsIPresShell.h that doesn't compile without some other header being included first, we'll notice.
Comment 14 Ed Morley [:emorley] 2011-06-30 03:06:47 PDT
(In reply to comment #13)
> I consider putting this include first to be bad practice. 

The logic makes sense, thanks for the feedback. I did check the MDN coding style page (https://developer.mozilla.org/En/Developer_Guide/Coding_Style) but there was nothing on this; though presume if I had more C++ experience I would do it like that out of habit anyway.

I'll address this + the fact that two more PR_* macros have cropped up in the meantime (in new code) in a follow-up patch.
Comment 15 Ed Morley [:emorley] 2011-06-30 06:50:03 PDT
Created attachment 543119 [details] [diff] [review]
Loose ends follow-up

After the main patch, there were still a couple of PR_(MAX|MIN|ABS|ROUNDUP) macro calls left, including some that have been introduced in new code since the patch (eg http://hg.mozilla.org/mozilla-central/diff/99c270809649/layout/base/nsBidiPresUtils.cpp). 

This takes care of the stragglers & reorders the includes in nsPresShell.cpp per comment 13.

I'm also going to add a section to the coding style MDN wiki page, to avoid further instances of new code adding yet more of these macros back in.
Comment 16 Ed Morley [:emorley] 2011-06-30 11:36:36 PDT
Added section to the coding style MDN wiki page, here:
https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Usage_of_PR_(MAX.7cMIN.7cABS.7cROUNDUP)_macro_calls

Feel free to adjust/correct wording/location/styling as needed.

Simon, CCing you so you are aware of the changes here, since your patch in bug 665837 added more PR_MIN calls after this bug removed the rest.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-30 15:00:51 PDT
Comment on attachment 543119 [details] [diff] [review]
Loose ends follow-up

Review of attachment 543119 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 18 Ed Morley [:emorley] 2011-07-03 03:47:05 PDT
Comment on attachment 543119 [details] [diff] [review]
Loose ends follow-up

http://hg.mozilla.org/mozilla-central/rev/01e79adc8c0f

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