499995 - fix signed/unsigned compiler warnings in mailnews

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[timeless]

Attachment: fix signed/unsigned warnings

there are a bunch of signed/unsigned warnings in mailnews. i've had this patch in my tree for a while and hadn't gotten around to posting it....

Comment 1

[[e-mail]]

Comment on attachment 384665 [details] [diff] [review]
fix signed/unsigned warnings

TMI

OK, so the nsTArray.NoIndex changes and the unused variable removals I can easily rubber-stamp if you want to give me a separate patch.

I'm always concerned about casts in conditions, as your lack of context means I'll have to look up the source of the LHS and RHS and work out whether the cast makes sense. Changing the type of a loop index variable in particular is always nicer since it's probably likely to be the correct fix anyway.

You can't use kNotFound because that's part of the internal string API, and we don't want to depend on it.

Comment 2

[timeless]

Attachment: less of the bits

Comment 3

[[e-mail]]

Comment on attachment 384938 [details] [diff] [review]
less of the bits

>r=neil sr=neil
Except as noted below.

> NS_IMETHODIMP nsMsgGroupThread::GetChildKeyAt(PRInt32 aIndex, nsMsgKey *aResult)
> NS_IMETHODIMP nsMsgGroupThread::GetChildAt(PRInt32 aIndex, nsIMsgDBHdr **aResult)
All the callers pass in 0, 1 or a PRUint32, so it would be better to change the type of aIndex.

>+      if (keyIndex != m_origKeys.NoIndex) 
Nit: trailing space

>-    if (viewThread->HdrIndex(aHdrChanged) != -1)
>+    if (viewThread->HdrIndex(aHdrChanged) != nsTArray_base::NoIndex)
The method deliberately returns -1, and the other caller even stores the result in a PRInt32, so I think it would be better to change the return type.

>-const long kMaxDownloadTableSize = 500;
I thought Count() was signed...

>-mime_write_message_body(nsIMsgSend *state, const char *buf, PRInt32 size)
>+mime_write_message_body(nsIMsgSend *state, const char *buf, PRInt32 aSize)
I'd want to change size to PRUint32 but that might confuse MIME_QPEncoderInit.

Comment 4

[Justin Wood (:Callek)]

(In reply to comment #2)
> Created an attachment (id=384938) [details]
> less of the bits

bitrotted....

Comment 5

[timeless]

Attachment: updated [Checkin: Comment 7]

I'm not touching mime_write_message_body this round. I have a followon for one IMAP related thing which i didn't want to fit here too.

Comment 6

[[e-mail]]

Comment on attachment 423507 [details] [diff] [review]
updated
[Checkin: Comment 7]

This patch fixes 40 warnings in the files that it causes to be recompiled, out of 2468...

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 423507 [details] [diff] [review]
updated
[Checkin: Comment 7]


http://hg.mozilla.org/comm-central/rev/82285c09d260

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> http://hg.mozilla.org/comm-central/rev/82285c09d260

is causing:
{
Linux:
test_nsMsgDBView.js | test failed (with xpcshell return code: -11), see following log:

Windows:
command timed out: 300 seconds without output
program finished with exit code 1
}

Either that test needs to be updated too or there is something wrong with this changeset.
I should back it out, but, as MoMe TryServer does not run tests, I prefer to let someone more knowledgeable have a look and decide first.

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)

Interestingly:
It fails on all 3 platforms on TB 3.2.
It passes on Windows TB 3.1.
So it would look like an issue in or related to m-1.9.3...

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #8)
> I should back it out, but, as MoMe TryServer does not run tests, I prefer to
> let someone more knowledgeable have a look and decide first.

The rule is just the same with mozilla-central - that if there is a failure the person who checked it in is on the hook to either fix it or back it out.

Whether or not we have tests on try server does NOT matter. If we can't reproduce locally, then we'll figure out what to do.

Backed out:

http://hg.mozilla.org/comm-central/rev/8a9f59aea9f1

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #10)

> The rule is just the same with mozilla-central - that if there is a failure the
> person who checked it in is on the hook to either fix it or back it out.

Well, I was under the impression TB team was still most active on 3.0.x and not (3.1 nor) 3.2.

> Whether or not we have tests on try server does NOT matter. If we can't

It does matter: to keep the changeset in the tree was the only way (for me) to see how more/different builds behaved.

> reproduce locally, then we'll figure out what to do.

Well, now seeing which builds are failing, I wonder how that wasn't noticed BEFORE asking for checkin-needed :-<

> Backed out:

Thanks.

Comment 12

[:aceman]

Suyash, would you like to check what can be salvaged from this patch?

Comment 13

[Suyash Agarwal (:sshagarwal)]

Attachment: Cleaned up patch

Sorry for taking so long. This patch is a bit cleaned up version of the uploaded one. Few other minor changes here and there.

Comment 14

[:aceman]

Comment on attachment 8346016 [details] [diff] [review]
Cleaned up patch

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

I'm not competent enough to comment on which variables should be int or uint. Have you confirmed that the patch fixes some compiler warnings from gcc?

::: mailnews/base/public/nsIMsgThread.idl
@@ -16,1 @@
>    attribute nsMsgKey threadKey;

Is there any UUID in this interface that would need changing?

Comment 15

[Suyash Agarwal (:sshagarwal)]

Attachment: cleaned up patch 1.1

Sorry, I had that noted, just forgot to make the change.
And, I think, the compiler warnings about signed/unsigned comparison reduced somewhat, not sure though.

Comment 16

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8346091 [details] [diff] [review]
cleaned up patch 1.1

May I put my name in the patch header? :)

Comment 17

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #16)
> May I put my name in the patch header? :)

You can update the header to have proper hg format, you can put your name there as usual, but in the patch description line put "(original version by timeless)" at the end :) I think that is how it is done if you want to give credit to the original author.

Comment 18

[[e-mail]]

Comment on attachment 8346091 [details] [diff] [review]
cleaned up patch 1.1

>-  int32_t                 mChildIndex;
>+  uint32_t                mChildIndex;
...
>           mChildIndex = MsgKeyFirstChildIndex(msgKey);
>-          mDone = (mChildIndex < 0);
>+          mDone = (mChildIndex <= 0);
This is wrong because MsgKeyFirstChildIndex returns an int32_t which is currently allowed to be -1.

>-NS_IMETHODIMP nsMsgXFGroupThread::GetChildHdrAt(int32_t aIndex, nsIMsgDBHdr **aResult)
>+NS_IMETHODIMP nsMsgXFGroupThread::GetChildHdrAt(uint32_t aIndex, nsIMsgDBHdr **aResult)
> {
>-  if (aIndex < 0 || aIndex >= m_folders.Count())
>+  if (aIndex >= m_folders.Count())
m_folders.Count() returns an int32_t, did you mean m_folders.Length()?

>-  int32_t                        mTaskIndex;
>+  uint32_t                       mTaskIndex;
Won't this lone change introduce more warnings than it fixes?

>-int32_t nsMsgXFViewThread::HdrIndex(nsIMsgDBHdr *hdr)
>+uint32_t nsMsgXFViewThread::HdrIndex(nsIMsgDBHdr *hdr)
Again, this can return -1.

>-  int32_t folderIndex = m_destFolders.IndexOf(folder);
>+  uint32_t folderIndex = m_destFolders.IndexOf(folder);
Again, IndexOf can return -1.

>-  int32_t                 mChildIndex;
>+  uint32_t                 mChildIndex;
This is wrong for the same reason it was for nsMsgGroupThread, except here the test didn't even get changed.

>-  int32_t head_buffer_fp;    /* Active length. */
>-  int32_t head_buffer_size;    /* How big it is. */
>+  uint32_t head_buffer_fp;    /* Active length. */
>+  uint32_t head_buffer_size;    /* How big it is. */
Again, this lone change probably introduces more warnings.

Comment 19

[Suyash Agarwal (:sshagarwal)]

(In reply to [e-mail] from comment #18)
> Comment on attachment 8346091 [details] [diff] [review]
> cleaned up patch 1.1
Thanks for the review.
> 
> >-  int32_t                 mChildIndex;
> >+  uint32_t                mChildIndex;
> ...
> >           mChildIndex = MsgKeyFirstChildIndex(msgKey);
> >-          mDone = (mChildIndex < 0);
> >+          mDone = (mChildIndex <= 0);
> This is wrong because MsgKeyFirstChildIndex returns an int32_t which is
> currently allowed to be -1.

> >-NS_IMETHODIMP nsMsgXFGroupThread::GetChildHdrAt(int32_t aIndex, nsIMsgDBHdr **aResult)
> >+NS_IMETHODIMP nsMsgXFGroupThread::GetChildHdrAt(uint32_t aIndex, nsIMsgDBHdr **aResult)
> > {
> >-  if (aIndex < 0 || aIndex >= m_folders.Count())
> >+  if (aIndex >= m_folders.Count())
> m_folders.Count() returns an int32_t, did you mean m_folders.Length()?
> 
> >-int32_t nsMsgXFViewThread::HdrIndex(nsIMsgDBHdr *hdr)
> >+uint32_t nsMsgXFViewThread::HdrIndex(nsIMsgDBHdr *hdr)
> Again, this can return -1.
> 
> >-  int32_t folderIndex = m_destFolders.IndexOf(folder);
> >+  uint32_t folderIndex = m_destFolders.IndexOf(folder);
> Again, IndexOf can return -1.
For all these errors in my patch, what would you like me to do?
Change the function calls so that we have correct throwers that throw uint32_t now or upcast all the catchers to uint32_t so that we don't have lossy assignment warning?

I am able to understand the cause of functions IndexOf(), Count() returning -1 to indicate that this isn't a valid value. Can't we return some other very large value to mark this same scenario? I think this way it will be logically correct as an Index can never be negative so the function will have an unsigned int as its return value etc.

Comment 20

[:aceman]

Changing indexOf is probably unwanted as it is spread and known throughout whole mozilla code that it returns -1. Yeah, sometimes cast as uint32_t(-1).

It seem the proper way forward here is to not change the variables to uint, but keep them as int and convert the other variables (that are being compared to) to int. Or, just cast them to uint32_t in some specific comparision expressions where needed, e.g. "if (aIndex (int32_t)>= m_folders.Count())" (but I do not say aIndex should be changed to uint32_t in the function declaration as you do). Or you could change .Count() to .Length() where appropriate as Neil shows.

Comment 21

[[e-mail]]

I suppose MsgKeyFirstChildIndex could be tweaked to return numChildren instead of -1, although you would have to fix the check appropriately.

HdrIndex could be tweaked to return nsMsgVieWIndex_None instead of -1. However you didn't actually change the caller so I'm not sure what the change there is about in the first place.

I don't know what the folderIndex change is trying to achieve.

Comment 22

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1.5

Made the changes at a few places.

Comment 23

[Suyash Agarwal (:sshagarwal)]

(In reply to [e-mail] from comment #18)
> >-  int32_t head_buffer_fp;    /* Active length. */
> >-  int32_t head_buffer_size;    /* How big it is. */
> >+  uint32_t head_buffer_fp;    /* Active length. */
> >+  uint32_t head_buffer_size;    /* How big it is. */
> Again, this lone change probably introduces more warnings.
I looked for the code areas that are using these variables, and I didn't find any need for using int32_t. So, I hope this can remain. Rest is what you say.

Thanks.

Comment 24

[[e-mail]]

(In reply to Suyash Agarwal from comment #23)
> (In reply to comment #18)
> > >-  int32_t head_buffer_fp;    /* Active length. */
> > >-  int32_t head_buffer_size;    /* How big it is. */
> > >+  uint32_t head_buffer_fp;    /* Active length. */
> > >+  uint32_t head_buffer_size;    /* How big it is. */
> > Again, this lone change probably introduces more warnings.
> I looked for the code areas that are using these variables, and I didn't
> find any need for using int32_t. So, I hope this can remain.

Heh, I compiled your latest patch, and this is the only change that actually fixes any of the warnings...

Comment 25

[:aceman]

Will there by any changes to the patch or is it ready for check in?

Comment 26

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #25)
> Will there by any changes to the patch or is it ready for check in?

I am not sure. As Neil sir said in his comment 24 that there is only a minor change in the patch that reduces some warnings so, possibly I need to make more changes to reduce more warnings, I think.

Comment 27

[[e-mail]]

(In reply to aceman from comment #25)
> Will there be any changes to the patch or is it ready for check in?

I'm not requiring any changes.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/5609327aa568