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

RESOLVED FIXED in Thunderbird 51.0

Status

MailNews Core
Import
--
trivial
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: Stefan Sitter, Assigned: Stefan Sitter)

Tracking

Thunderbird 51.0
x86
Windows

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8670459 [details] [diff] [review]
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.
Attachment #8670459 - Flags: review?(Pidgeot18)
(Assignee)

Comment 2

2 years ago
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

2 years ago
Only one reviewer is sufficient.

Comment 4

9 months ago
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().
Attachment #8670459 - Flags: review?(Pidgeot18) → review?(jorgk)

Updated

9 months ago
OS: Unspecified → Windows
Hardware: Unspecified → x86

Comment 5

9 months ago
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++)

Updated

9 months ago
Flags: needinfo?(ssitter)

Comment 6

9 months ago
(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

9 months ago
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

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

I'll make it ULONG and land this now. Thanks.
Flags: needinfo?(ssitter)
Attachment #8670459 - Flags: review?(jorgk) → review+

Comment 9

9 months ago
https://hg.mozilla.org/comm-central/rev/0835213d973d
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
(Assignee)

Comment 10

9 months ago
(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

9 months ago
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

9 months ago
Created attachment 8780831 [details] [diff] [review]
Follow up patch.

If we care enough we can fix it like this.
Attachment #8780831 - Flags: review?(acelists)

Comment 13

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

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

OK
Attachment #8780831 - Flags: review?(acelists) → review+

Comment 14

9 months ago
https://hg.mozilla.org/comm-central/rev/f5bca259cd8e192e15618e71f9c68f963577c434
You need to log in before you can comment on or make changes to this bug.