Last Comment Bug 720524 - use PRInt32 for variable containing return value of FindChar in nsMsgDBFolder.cpp
: use PRInt32 for variable containing return value of FindChar in nsMsgDBFolder...
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: x86 Linux
-- trivial (vote)
: Thunderbird 12.0
Assigned To: :aceman
Depends on:
  Show dependency treegraph
Reported: 2012-01-23 15:11 PST by :aceman
Modified: 2012-01-27 06:27 PST (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch (1.87 KB, patch)
2012-01-23 15:22 PST, :aceman
no flags Details | Diff | Splinter Review
patch v2 (1.51 KB, patch)
2012-01-26 12:21 PST, :aceman
neil: review+
Details | Diff | Splinter Review

Description User image :aceman 2012-01-23 15:11:32 PST
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:

I suggest to change this place to be sure.
Comment 1 User image :aceman 2012-01-23 15:22:56 PST
Created attachment 590891 [details] [diff] [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 2 User image Ludovic Hirlimann [:Usul] 2012-01-24 01:10:24 PST
Comment on attachment 590891 [details] [diff] [review]

C++ backend code found in mailnews , either Bienvenu, Neil or Standard8
Comment 3 User image David :Bienvenu 2012-01-24 09:42:18 PST
Comment on attachment 590891 [details] [diff] [review]

switching request to Neil - I'm not sure there's a reason to use static_cast for this kind of cast.
Comment 4 User image 2012-01-24 16:22:35 PST
Could we avoid the casts by switching all three variables to PRInt32?
Comment 5 User image :aceman 2012-01-24 23:55:26 PST
I think yes. I just wanted to preserve the PRUints to show those variables can only be positive.
Comment 6 User image 2012-01-25 16:15:50 PST
(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 for example.
Comment 7 User image :aceman 2012-01-26 12:21:23 PST
Created attachment 591892 [details] [diff] [review]
patch v2

No problem.
Comment 8 User image 2012-01-26 13:01:27 PST
Comment on attachment 591892 [details] [diff] [review]
patch v2

Well, that was easy :-)
Comment 9 User image Mark Banner (:standard8) 2012-01-27 06:27:27 PST
Checked in:

Note You need to log in before you can comment on or make changes to this bug.