661584 - Code cleanup, substitute PR_(MAX|MIN|ABS|ROUNDUP) macro calls

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

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

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

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

Attachment: 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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

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

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) (email my personal email if necessary)]

PRUint32(-NS_PTR_TO_INT32(aSource) & 0xF)

Comment 8

[Ed Morley [:emorley]]

http://dev.philringnalda.com/tbpl/?tree=Try&rev=a03554955ed0

The changes from comment 6 (not sure if they need re-review or not) :

http://hg.mozilla.org/try/diff/72e9bf93551a/content/base/src/nsTextFragment.cpp

http://hg.mozilla.org/try/diff/72e9bf93551a/content/base/src/nsTextFragmentSSE2.cpp

http://hg.mozilla.org/try/diff/72e9bf93551a/xpcom/string/src/nsUTF8UtilsSSE2.cpp

Once the ssl tunnel patch is reviewed, will qfold everything and upload the updated patch.

Comment 9

[(not currently active) Ted Mielczarek]

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

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

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/fc776fa4afb7

Comment 12

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/fc776fa4afb7

Comment 13

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

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

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

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

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) (email my personal email if necessary)]

Comment on attachment 543119 [details] [diff] [review]
Loose ends follow-up

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

Comment 18

[Ed Morley [:emorley]]

Comment on attachment 543119 [details] [diff] [review]
Loose ends follow-up

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