Closed Bug 720524 Opened 12 years ago Closed 12 years ago

use PRInt32 for variable containing return value of FindChar in nsMsgDBFolder.cpp

Categories

(MailNews Core :: Backend, defect)

x86
Linux
defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 12.0

People

(Reporter: aceman, Assigned: aceman)

Details

Attachments

(1 file, 1 obsolete file)

mailnews/base/util/nsMsgDBFolder.cpp: In member function 'void nsMsgDBFolder::compressQuotesInMsgSnippet(const nsString&, nsAString_internal&)':
mailnews/base/util/nsMsgDBFolder.cpp:5702:25: warning: comparison between signed and unsigned integer expressions

This is the relevant code:

  PRUint32 lineFeedPos = 0;
  while (offset < msgBodyStrLen)
  {
    lineFeedPos = aMsgSnippet.FindChar('\n', offset);
    if (lineFeedPos != -1)

What happens if FindChar returns -1? Can we be sure this will work (converted to some big signed value) on all compilers/systems?

I think most places where this function is used use PRInt32:
http://mxr.mozilla.org/comm-central/ident?i=FindChar

I suggest to change this place to be sure.
Summary: use PRInt32 for variable containig return value of FindChar in nsMsgDBFolder.cpp → use PRInt32 for variable containing return value of FindChar in nsMsgDBFolder.cpp
Attached patch patch (obsolete) — Splinter Review
After changing to PRInt32 the compiler warning moved to line 5711. So I added a cast there, which should be safe (the variable must have positive value in that line).

Who can review this kind of issue?
Comment on attachment 590891 [details] [diff] [review]
patch

C++ backend code found in mailnews , either Bienvenu, Neil or Standard8
Attachment #590891 - Flags: review?(dbienvenu)
Comment on attachment 590891 [details] [diff] [review]
patch

switching request to Neil - I'm not sure there's a reason to use static_cast for this kind of cast.
Attachment #590891 - Flags: review?(dbienvenu) → review?(neil)
Could we avoid the casts by switching all three variables to PRInt32?
I think yes. I just wanted to preserve the PRUints to show those variables can only be positive.
(In reply to :aceman from comment #5)
> I just wanted to preserve the PRUints to show those variables can only be positive.
I read somewhere that you should avoid unsigned variables, as they come with extra guarantees (such as modular arithmetic) which reduces the potential for compiler optimisation. See also http://blogs.msdn.com/b/laurionb/archive/2009/03/27/unsigned-considered-harmful.aspx for example.
Attached patch patch v2Splinter Review
No problem.
Attachment #590891 - Attachment is obsolete: true
Attachment #590891 - Flags: review?(neil)
Attachment #591892 - Flags: review?(neil)
Comment on attachment 591892 [details] [diff] [review]
patch v2

Well, that was easy :-)
Attachment #591892 - Flags: review?(neil) → review+
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/c335daff820f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 12.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: