Closed Bug 210608 Opened 16 years ago Closed 7 years ago

build warnings in nsImapMailFolder

Categories

(MailNews Core :: Networking: IMAP, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 19.0

People

(Reporter: bugzilla, Assigned: aceman)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

whenever I build mail&news imap:

c:/mozilla/mozilla/mailnews/imap/src/nsImapMailFolder.cpp: In member function
   `virtual nsresult nsImapMailFolder::Rename(const PRUnichar*,
   nsIMsgWindow*)':
c:/mozilla/mozilla/mailnews/imap/src/nsImapMailFolder.cpp:1302: warning: cast
   to pointer from integer of different size
c:/mozilla/mozilla/mailnews/imap/src/nsImapMailFolder.cpp: In member function
   `virtual nsresult nsImapMailFolder::NormalEndMsgWriteStream(unsigned int,
   int, nsIImapUrl*)':
c:/mozilla/mozilla/mailnews/imap/src/nsImapMailFolder.cpp:3755: warning: unused
   variable `PRBool needMsgID'
Attached patch patch (obsolete) — Splinter Review
fixes warnings, removes unnecessary myCString.Truncate() in ctor,
s/-1/kNotFound/
Attachment #126451 - Flags: superreview?(bienvenu)
Attachment #126451 - Flags: review?(bienvenu)
taking
Assignee: bienvenu → dwitte
I'll try this patch and if it's OK, I'll check it in. Thx for the patch.
wait a sec... I think this patch is also wrong. :/

the format string that uses &m_hierarchyDelimiter needs to be null-terminated,
right? we'll need a strdup there or something.

new patch coming up...
Attachment #126451 - Flags: superreview?(bienvenu)
Attachment #126451 - Flags: review?(bienvenu)
Attached patch patch v2 (obsolete) — Splinter Review
ugh... so the original code was correct. I put a comment there and used
reinterpret_cast so folks won't be trapped by this in future.

thanks ;)
Attachment #126451 - Attachment is obsolete: true
Attachment #126458 - Flags: superreview?(bienvenu)
Blocks: buildwarning
bienvenu: just a reminder... any chance of r+sr here if you've got a few extra
mins? thx!
Comment on attachment 126458 [details] [diff] [review]
patch v2

rotating request to mscott.
Attachment #126458 - Flags: superreview?(mscott)
Attachment #126458 - Flags: superreview?(bienvenu)
Attachment #126458 - Flags: review?(mscott)
Product: MailNews → Core
Comment on attachment 126458 [details] [diff] [review]
patch v2

two years old, is this patch still valid? If so, re-request but review for build warnings tend to be low on my priority list. (But not two years low!)
Attachment #126458 - Flags: superreview?(mscott)
Attachment #126458 - Flags: review?(mscott)
-> default owner
Assignee: dwitte → bienvenu
QA Contact: grylchan → networking.imap
Product: Core → MailNews Core
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090417 SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/9e06217fc471
 +http://hg.mozilla.org/comm-central/rev/...)

{
nsImapMailFolder.cpp(2331) : warning C4018: '<' : signed/unsigned mismatch
nsImapMailFolder.cpp(2506) : warning C4018: '<' : signed/unsigned mismatch
nsImapMailFolder.cpp(2653) : warning C4018: '>' : signed/unsigned mismatch
nsImapMailFolder.cpp(2564) : warning C4101: 'created' : unreferenced local variable
nsImapMailFolder.cpp(2746) : warning C4018: '<' : signed/unsigned mismatch
nsImapMailFolder.cpp(3820) : warning C4018: '<' : signed/unsigned mismatch
nsImapMailFolder.cpp(6992) : warning C4018: '>=' : signed/unsigned mismatch
}
Depends on: 90906
Blocks: 90906
No longer depends on: 90906
bienvenu, does this still reproduce?
Gary, does this still apply? (it'd be a miracle)
timeless, you up for taking on another compiler warning?
Comment on attachment 126458 [details] [diff] [review]
patch v2

yeah, it's obsolete
Attachment #126458 - Attachment is obsolete: true
Assignee: dbienvenu → nobody
do these warnings still occur?
Flags: needinfo?(acelists)
Attached patch patch v3 (obsolete) — Splinter Review
Those that Serge posted are valid. I attach a patch to fix them.
I could not fix the:
mailnews/imap/src/nsImapMailFolder.cpp: In member function 'virtual nsresult nsImapMailFolder::Rename(const nsAString_internal&, nsIMsgWindow*)':
mailnews/imap/src/nsImapMailFolder.cpp:1643:31: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
fro the original report. The solution in the old patch does not work anymore.
Maybe irving knows how to fix that.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #673615 - Flags: review?
Attachment #673615 - Flags: feedback?(irving)
Flags: needinfo?(acelists)
OS: Windows 2000 → All
Hardware: x86 → All
Attachment #673615 - Flags: review?(irving)
Attachment #673615 - Flags: review?
Attachment #673615 - Flags: feedback?(irving)
Oh, I messed the requests a bit. But I intentionally requested feedback only meaning if somebody knows how to add the "cast to pointer from integer of different size"-fix to the patch. But if there is no proposal, then the patch can be reviewed as is.
(In reply to :aceman from comment #14)

> I could not fix the:
> mailnews/imap/src/nsImapMailFolder.cpp: In member function 'virtual nsresult
> nsImapMailFolder::Rename(const nsAString_internal&, nsIMsgWindow*)':
> mailnews/imap/src/nsImapMailFolder.cpp:1643:31: warning: cast to pointer
> from integer of different size [-Wint-to-pointer-cast]
> fro the original report. The solution in the old patch does not work anymore.
> Maybe irving knows how to fix that.

That's a medium-ugly bit of code; basically that array contains parameters that are eventually passed to a variable argument list printf()-like function, which expects all of its arguments to be void* sized. The character value being passed would be coerced silently if it was directly in the function call, but the coersion isn't silent (at least on gcc) because we're initializing an array with it instead.

You should be able to silence it with an extra cast to intptr_t, but you should do a try build to make sure it's available on all platforms.

--- a/mailnews/imap/src/nsImapMailFolder.cpp
+++ b/mailnews/imap/src/nsImapMailFolder.cpp
@@ -1640,7 +1640,7 @@ NS_IMETHODIMP nsImapMailFolder::Rename (
       {
         const PRUnichar *formatStrings[] =
         {
-           (const PRUnichar*) m_hierarchyDelimiter
+           (const PRUnichar*)(intptr_t)m_hierarchyDelimiter
         };
         nsString alertString;
         rv = bundle->FormatStringFromID(IMAP_SPECIAL_CHAR,
Comment on attachment 673615 [details] [diff] [review]
patch v3

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

The changes in this patch look fine, even if they do make me wish we weren't so inconsistent with signed/unsigned in our code...
Attachment #673615 - Flags: review?(irving) → review+
Attached patch patch v4Splinter Review
Attachment #673615 - Attachment is obsolete: true
Attachment #673958 - Flags: review+
Keywords: checkin-needed
Whiteboard: [please run a try-build on all platforms before checkin, per comment 16]
Try run for dc6b269ae8ca is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=dc6b269ae8ca
Results (out of 9 total builds):
    warnings: 8
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ireid@mozilla.com-dc6b269ae8ca
https://hg.mozilla.org/comm-central/rev/99507befeb52
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [please run a try-build on all platforms before checkin, per comment 16]
Target Milestone: --- → Thunderbird 19.0
You need to log in before you can comment on or make changes to this bug.