Replace PR_MIN/PR_MAX with NS_MIN/NS_MAX, in comm-central

VERIFIED FIXED in Thunderbird 11.0

Status

MailNews Core
Backend
--
minor
VERIFIED FIXED
8 years ago
6 years ago

People

(Reporter: sgautherie, Assigned: standard8)

Tracking

Trunk
Thunderbird 11.0
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
+++ This bug was initially created as a clone of Bug #512106 +++

See that bug about why and how to do it...
(Reporter)

Updated

8 years ago
Whiteboard: [good first bug]

Comment 1

8 years ago
Created attachment 402972 [details] [diff] [review]
Replace PR_MIN/PR_MAX with NS_MIN/NS_MAX in comm-central
(Reporter)

Comment 2

8 years ago
(In reply to comment #1)
> Created an attachment (id=402972) [details]
> Replace PR_MIN/PR_MAX with NS_MIN/NS_MAX in comm-central

I pushed it to TryServer...
Assignee: nobody → lusian
Status: NEW → ASSIGNED
(Reporter)

Updated

8 years ago
Attachment #402972 - Flags: superreview?(bugzilla)
Attachment #402972 - Flags: review?(bugzilla)
(Reporter)

Comment 3

8 years ago
Comment on attachment 402972 [details] [diff] [review]
Replace PR_MIN/PR_MAX with NS_MIN/NS_MAX in comm-central


(In reply to comment #2)
> I pushed it to TryServer...

Passed as '518504-comm-central__again'.
Comment on attachment 402972 [details] [diff] [review]
Replace PR_MIN/PR_MAX with NS_MIN/NS_MAX in comm-central

Sorry for the delay in getting to this, I've been very busy with Thunderbird 3 specifics the last couple of months.

>-  PRUint32 maxHdrs= PR_MIN(nsMsgSearchAttrib::OtherHeader + numHeaders+1, nsMsgSearchAttrib::kNumMsgSearchAttributes);
>+  PRUint32 maxHdrs= NS_MIN(nsMsgSearchAttrib::OtherHeader + numHeaders+1, (PRUint32)nsMsgSearchAttrib::kNumMsgSearchAttributes);

nit: I know this was just like it was, but please insert a space before the equals, and a space either side of the + in the numHeaders+1 bit.

>-        rv =  aOutStream->WriteFrom(mInStream, PR_MIN(avail, 4096), &bytesWritten);
>+        rv =  aOutStream->WriteFrom(mInStream, NS_MIN(avail, 4096U), &bytesWritten);

nit: please drop the second space after equals.

>diff --git a/mailnews/imap/src/nsImapMailFolder.cpp b/mailnews/imap/src/nsImapMailFolder.cpp

>-            rv = outputStream->Write(inputBuffer, PR_MIN((PRInt32) bytesRead, bytesLeft), &bytesWritten);
>-            NS_ASSERTION((PRInt32) bytesWritten == PR_MIN((PRInt32) bytesRead, bytesLeft), "wrote out incorrect number of bytes");
>+            rv = outputStream->Write(inputBuffer, NS_MIN((PRInt32) bytesRead, bytesLeft), &bytesWritten);
>+            NS_ASSERTION((PRInt32) bytesWritten == NS_MIN((PRInt32) bytesRead, bytesLeft), "wrote out incorrect number of bytes");

This bitrotted but I fixed it locally.

r/sr=Standard8

Thanks for doing the patch. Normally I'd get you to address the nits, but as I feel rotten for letting this hang for so long, I'll check it in in a few minutes.
Attachment #402972 - Flags: superreview?(bugzilla)
Attachment #402972 - Flags: superreview+
Attachment #402972 - Flags: review?(bugzilla)
Attachment #402972 - Flags: review+
Created attachment 413065 [details] [diff] [review]
[checked in] The fix v2

The patch with my comments address.

Checked in:
http://hg.mozilla.org/comm-central/rev/fb7d28871960
Attachment #402972 - Attachment is obsolete: true
Attachment #413065 - Flags: superreview+
Attachment #413065 - Flags: review+
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1a1
(Reporter)

Comment 6

8 years ago
(In reply to comment #5)
> http://hg.mozilla.org/comm-central/rev/fb7d28871960

{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1258555755.1258556056.751.gz
Linux x86-64 comm-central-trunk build on 2009/11/18 06:49:15
	
/builds/slave/comm-central-trunk-linux64/build/mailnews/base/src/nsMsgBiffManager.cpp: In member function ‘nsresult nsMsgBiffManager::SetNextBiffTime(nsBiffEntry&, nsTime)’:
/builds/slave/comm-central-trunk-linux64/build/mailnews/base/src/nsMsgBiffManager.cpp:256: error: no matching function for call to ‘NS_MIN(PRInt64&, long long int)’
}
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [good first bug]
I've backed out the change in just that file:

http://hg.mozilla.org/comm-central/rev/56e7f2b3ab98

I think I know how to fix this properly but want to investigate a bit more first.

Comment 8

8 years ago
Still error in
comm-central/mailnews/imap/src/nsImapUrl.cpp: In member function 'virtual nsresult nsImapUrl::GetListOfMessageIds(nsACString_internal&)':
comm-central/mailnews/imap/src/nsImapUrl.cpp:308: error: no matching function for call to 'NS_MIN(PRInt32&, long int)'
(In reply to comment #8)
> Still error in
> comm-central/mailnews/imap/src/nsImapUrl.cpp: In member function 'virtual
> nsresult nsImapUrl::GetListOfMessageIds(nsACString_internal&)':
> comm-central/mailnews/imap/src/nsImapUrl.cpp:308: error: no matching function
> for call to 'NS_MIN(PRInt32&, long int)'

Checked in a bustage fix for this bit:
http://hg.mozilla.org/comm-central/rev/3d346eb49f64

Still need to fix up the other bit I backed out.
http://mxr.mozilla.org/comm-central/search?string=PR_M%28IN%7CAX%29%5C%28&regexp=on&case=on&find=%2Fmailnews%2F.*%5C.cpp%24

...shows just one instance left in comm-central:
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgBiffManager.cpp#294
(the part backed out in comment 6 / comment 7.

Presume something like
> jitter = NS_MAX<PRInt64>(1000000LL, NS_MIN<PRInt64>(jitter, 30000000LL));
...would fix the build error.

Don't have a comm-central repo checked out, so can't sort myself.

Once this bug is closed, all of comm-central and also mozilla-central are PR_M(IN|AX) free :-)
Assignee: lusian → mbanner
Sorry, I don't have time to fix this up at the moment.
Assignee: mbanner → nobody
Keywords: helpwanted

Comment 12

6 years ago
Created attachment 574391 [details] [diff] [review]
patch according to comment 10

I didn't get a build problem even with the original version (on linux 32bit, gcc 4.5), but here is the update according to comment 10. Maybe it is needed on 64bit.
Attachment #574391 - Flags: review?(mbanner)
Attachment #574391 - Flags: review?(mbanner) → review+
Keywords: checkin-needed

Updated

6 years ago
Keywords: helpwanted
Checked in: http://hg.mozilla.org/comm-central/rev/e9a00fadaf6c
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

6 years ago
Target Milestone: Thunderbird 3.1a1 → Thunderbird 11.0

Updated

6 years ago
Target Milestone: Thunderbird 11.0 → Thunderbird 3.1a1
(Reporter)

Comment 14

6 years ago
V.Fixed, per MXR.
Assignee: nobody → mbanner
Status: RESOLVED → VERIFIED
Flags: in-testsuite-
Target Milestone: Thunderbird 3.1a1 → Thunderbird 11.0
You need to log in before you can comment on or make changes to this bug.