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

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[:aceman]

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.

Comment 1

[:aceman]

Attachment: patch

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

[Ludovic Hirlimann [:Usul]]

Comment on attachment 590891 [details] [diff] [review]
patch

C++ backend code found in mailnews , either Bienvenu, Neil or Standard8

Comment 3

[David :Bienvenu]

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.

Comment 4

[[e-mail]]

Could we avoid the casts by switching all three variables to PRInt32?

Comment 5

[:aceman]

I think yes. I just wanted to preserve the PRUints to show those variables can only be positive.

Comment 6

[[e-mail]]

(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.

Comment 7

[:aceman]

Attachment: patch v2

No problem.

Comment 8

[[e-mail]]

Comment on attachment 591892 [details] [diff] [review]
patch v2

Well, that was easy :-)

Comment 9

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/c335daff820f