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

RESOLVED FIXED in Firefox 7

Status

()

Firefox
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: emorley)

Tracking

({dev-doc-complete})

Trunk
Firefox 7
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Updated

6 years ago
Assignee: nobody → bmo
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
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
Attachment #539576 - Flags: review?(roc)
(Assignee)

Comment 2

6 years ago
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.
Attachment #539579 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 3

6 years ago
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.
Attachment #539580 - Flags: review?(roc)
Comment on attachment 539576 [details] [diff] [review]
The simple s/PR_foo/NS_foo cases

Review of attachment 539576 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #539576 - Flags: review?(roc) → review+
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>
Attachment #539580 - Flags: review?(roc) → review+
(Assignee)

Comment 6

6 years ago
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!)
PRUint32(-NS_PTR_TO_INT32(aSource) & 0xF)
(Assignee)

Comment 8

6 years ago
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 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?
Attachment #539579 - Flags: review?(ted.mielczarek) → review+
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.
Attachment #539576 - Attachment is obsolete: true
Attachment #539579 - Attachment is obsolete: true
Attachment #539580 - Attachment is obsolete: true
Attachment #541926 - Flags: review+
(Reporter)

Comment 11

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/fc776fa4afb7
Flags: in-testsuite-
Whiteboard: [inbound]
Target Milestone: --- → Firefox 7
Merged:
http://hg.mozilla.org/mozilla-central/rev/fc776fa4afb7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
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.
(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.
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.
Attachment #543119 - Flags: review?(roc)
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.
(Assignee)

Updated

6 years ago
Keywords: dev-doc-complete
Comment on attachment 543119 [details] [diff] [review]
Loose ends follow-up

Review of attachment 543119 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #543119 - Flags: review?(roc) → review+
Comment on attachment 543119 [details] [diff] [review]
Loose ends follow-up

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

Updated

6 years ago
Blocks: 518502
You need to log in before you can comment on or make changes to this bug.