1212068 - Fix "warning C4018: '<': signed/unsigned mismatch" in mailnews/import and mailnews/mapi

Status: RESOLVED FIXED

(MailNews Core :: Import, defect)

Description

[Stefan Sitter [:ssitter]]

> mailnews/import/outlook/src/nsOutlookMail.cpp(652): warning C4018: '<': signed/unsigned mismatch
> mailnews/mapi/mapihook/src/msgMapiHook.cpp(720): warning C4018: '<': signed/unsigned mismatch
> mailnews/mapi/mapihook/src/msgMapiImp.cpp(666): warning C4018: '<': signed/unsigned mismatch

Comment 1

[Stefan Sitter [:ssitter]]

Attachment: Fix warning C4018: '<': signed/unsigned mismatch

> mailnews/import/outlook/src/nsOutlookMail.cpp(652): warning C4018: '<': signed/unsigned mismatch
Change type to uint32_t to match sa->cValues and the required target type in AddListCardColumnsToRow()

> mailnews/mapi/mapihook/src/msgMapiHook.cpp(720): warning C4018: '<': signed/unsigned mismatch
Cast to int32_t to match the type returned by nsString.Find() to silence warning

> mailnews/mapi/mapihook/src/msgMapiImp.cpp(666): warning C4018: '<': signed/unsigned mismatch
Change type to size_t to match the type returned and consumed by nsTArray<nsCString>

Remove some unused variables reported in the touched files.

With this patch I now get 0 compiler warnings for mailnews/ with VS2015.

Comment 2

[Stefan Sitter [:ssitter]]

I'd like to ask you to tell me if I have to request review from a different reviewer for the Mapi part of the patch because I only chose the one that was suggested by Bugzilla for the Import component.

Comment 3

[Kent James (:rkent)]

Only one reviewer is sufficient.

Comment 4

[:aceman]

Comment on attachment 8670459 [details] [diff] [review]
Fix warning C4018: '<': signed/unsigned mismatch

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

::: mailnews/import/outlook/src/nsOutlookMail.cpp
@@ +640,5 @@
>    }
>  
>    LPENTRYID    lpEid;
>    ULONG        cbEid;
> +  uint32_t       idx;

Why not ULONG? It matches the variable being compared to exactly.

::: mailnews/mapi/mapihook/src/msgMapiImp.cpp
@@ +662,5 @@
>    ExtractAllAddresses(recipients, UTF16ArrayAdapter<>(names),
>      UTF16ArrayAdapter<>(addresses));
>    
> +  size_t numAddresses = names.Length();
> +  for (size_t i = 0; i < numAddresses; i++)

Would size_type work? That is the type returned by nsTArray.Length().

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8670459 [details] [diff] [review]
Fix warning C4018: '<': signed/unsigned mismatch

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

Stefan, can you answer these questions and then we land this.

::: mailnews/mapi/mapihook/src/msgMapiImp.cpp
@@ +662,5 @@
>    ExtractAllAddresses(recipients, UTF16ArrayAdapter<>(names),
>      UTF16ArrayAdapter<>(addresses));
>    
> +  size_t numAddresses = names.Length();
> +  for (size_t i = 0; i < numAddresses; i++)

Aceman, you have this in your patch:
for (uint32_t i = 0; i < numAddresses; i++)

Comment 6

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #5)
> > +  size_t numAddresses = names.Length();
> > +  for (size_t i = 0; i < numAddresses; i++)
> 
> Aceman, you have this in your patch:
> for (uint32_t i = 0; i < numAddresses; i++)

Yes, because I didn't look up nsTArray, just matched i to numAddresses. But if size_type does not compile, size_t is fine too.

Comment 7

[Jorg K (CEST = GMT+2)]

Let's land this with size_t here and ULONG above:
https://msdn.microsoft.com/en-us/library/cc815398(v=office.12).aspx

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8670459 [details] [diff] [review]
Fix warning C4018: '<': signed/unsigned mismatch

I'll make it ULONG and land this now. Thanks.

Comment 9

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/0835213d973d

Comment 10

[Stefan Sitter [:ssitter]]

(In reply to :aceman from comment #4)
> ::: mailnews/import/outlook/src/nsOutlookMail.cpp
> @@ +640,5 @@
> > +  uint32_t       idx;
> 
> Why not ULONG? It matches the variable being compared to exactly.

See comment 1. I think I choose uint32_t because that type is used to capture sa->cValues in line 651 and the call to AddListCardColumnsToRow() expects uint32_t as well. But ULONG should work too unless you have platform where ULONG is 64-bit and sa->cValues is larger than UINT32_MAX. But in this case integer truncation would happen independent if my solution or your solution is used.

Comment 11

[Jorg K (CEST = GMT+2)]

ULONG is the datatype cValues, see
https://msdn.microsoft.com/en-us/library/cc815398(v=office.12).aspx
I'll add a patch do it 100% correctly.

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: Follow up patch.

If we care enough we can fix it like this.

Comment 13

[:aceman]

Comment on attachment 8780831 [details] [diff] [review]
Follow up patch.

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

OK

Comment 14

[:aceman]

https://hg.mozilla.org/comm-central/rev/f5bca259cd8e192e15618e71f9c68f963577c434