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

RESOLVED FIXED in Thunderbird 12.0

Status

MailNews Core
Backend
--
trivial
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 12.0
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.51 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 1

5 years ago
Created attachment 590891 [details] [diff] [review]
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 on attachment 590891 [details] [diff] [review]
patch

C++ backend code found in mailnews , either Bienvenu, Neil or Standard8
Attachment #590891 - Flags: review?(dbienvenu)

Comment 3

5 years ago
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)

Comment 4

5 years ago
Could we avoid the casts by switching all three variables to PRInt32?
(Assignee)

Comment 5

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

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 591892 [details] [diff] [review]
patch v2

No problem.
Attachment #590891 - Attachment is obsolete: true
Attachment #590891 - Flags: review?(neil)
Attachment #591892 - Flags: review?(neil)

Comment 8

5 years ago
Comment on attachment 591892 [details] [diff] [review]
patch v2

Well, that was easy :-)
Attachment #591892 - Flags: review?(neil) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/c335daff820f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.