Closed Bug 1256473 Opened 8 years ago Closed 8 years ago

netwerk/base/nsSocketTransport2.cpp(3029): warning C4838: conversion from 'int' to 'ULONG' requires a narrowing conversion

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: gps, Assigned: gps)

References

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 3 obsolete files)

This warning is turning into an error in automation when building with VS2015u1.
As part of unblocking building with VS2015u1 in automation, I'm mass
disabling compiler warnings that are turned into errors. This is not
the preferred mechanism to fix compilation warnings. So hopefully
this patch never lands because someone insists of fixing the underlying
problem instead. But if it does land, hopefully the workaround is
only temporary.

Review commit: https://reviewboard.mozilla.org/r/39855/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39855/
Attachment #8730420 - Flags: review?(odvarko)
As part of unblocking building with VS2015u1 in automation, I'm mass
disabling compiler warnings that are turned into errors. This is not
the preferred mechanism to fix compilation warnings. So hopefully
someone fixes the underlying problem someday. However, there are tons
of ignored warnings in security/certverifier, so I guess the workaround
in this patch is par for the course.

Review commit: https://reviewboard.mozilla.org/r/39865/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39865/
Attachment #8730420 - Attachment is obsolete: true
Attachment #8730420 - Flags: review?(odvarko)
As part of unblocking building with VS2015u1 in automation, I'm mass
disabling compiler warnings that are turned into errors. This is not
the preferred mechanism to fix compilation warnings. So hopefully
this patch never lands because someone insists of fixing the underlying
problem instead. But if it does land, hopefully the workaround is
only temporary.

Review commit: https://reviewboard.mozilla.org/r/39867/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39867/
Attachment #8730441 - Flags: review?(odvarko)
Attachment #8730439 - Attachment is obsolete: true
Comment on attachment 8730441 [details]
MozReview Request: Bug 1256473 - Disable C4838 to unblock compilation on VS2015; r?mayhemer

You want to ask Jan Bambas (:mayhemer) for review I guess.
Honza
Attachment #8730441 - Flags: review?(odvarko) → review-
Attachment #8730441 - Flags: review- → review?(honzab.moz)
Comment on attachment 8730441 [details]
MozReview Request: Bug 1256473 - Disable C4838 to unblock compilation on VS2015; r?mayhemer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39867/diff/1-2/
Attachment #8730441 - Attachment description: MozReview Request: Bug 1256473 - Disable C4838 to unblock compilation on VS2015; r?honza → MozReview Request: Bug 1256473 - Disable C4838 to unblock compilation on VS2015; r?mayhemer
Can you give me an example(s) in the code that is causing this warning?  Just to asses how big a problem we are hiding (if any).
Flags: needinfo?(gps)
The code in the bug summary is exhibiting the problem. Although the line number may have shifted.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27083abb8d05 should report the warning in <1hr.
Flags: needinfo?(gps)
The Try push reproduced the warning.
can you help move this forward mcmanus?
OK, seems like VS2015 uncovers a lot of problems we have.  The number of warnings you have to disable is, I could say, massive, when looking at bug 1124033 and its dependencies.

Tho, I don't much follow why conversion from int to ULONG should be explicitly narrowing on a 32 bit build.

Anyway, if this is the plan to support VS2015, then, however I'm nor super happy about it, no need to block here.
Attachment #8730441 - Flags: review?(honzab.moz) → review+
Comment on attachment 8730441 [details]
MozReview Request: Bug 1256473 - Disable C4838 to unblock compilation on VS2015; r?mayhemer

https://reviewboard.mozilla.org/r/39867/#review37557
I just see the one warning in that log..

:gps, do you mind seeing if this patch clears it up without the compiler directive? If not, I'm fine with adding the compiler switch

diff --git a/netwerk/base/nsSocketTransport2.cpp b/netwerk/base/nsSocketTransport2.cpp
index 34165ec..fcdf005 100644
--- a/netwerk/base/nsSocketTransport2.cpp
+++ b/netwerk/base/nsSocketTransport2.cpp
@@ -3017,20 +3017,20 @@ nsSocketTransport::PRFileDescAutoLock::SetKeepaliveVals(bool aEnabled,
                      mSocketTransport);
         return ErrorAccordingToNSPR(PR_GetError());
     }
 #endif
 
 #if defined(XP_WIN)
     // Windows allows idle time and retry interval to be set; NOT ping count.
     struct tcp_keepalive keepalive_vals = {
-        (int)aEnabled,
+        (u_long)aEnabled,
         // Windows uses msec.
-        aIdleTime * 1000,
-        aRetryInterval * 1000
+        (u_long)(aIdleTime * 1000),
+        (u_long)(aRetryInterval * 1000)
     };
     DWORD bytes_returned;
     int err = WSAIoctl(sock, SIO_KEEPALIVE_VALS, &keepalive_vals,
                        sizeof(keepalive_vals), NULL, 0, &bytes_returned, NULL,
                        NULL);
     if (NS_WARN_IF(err)) {
         LogOSError("nsSocketTransport WSAIoctl failed", mSocketTransport);
         return NS_ERROR_UNEXPECTED;
Assignee: nobody → gps
Whiteboard: [necko-active]
Attachment #8730441 - Attachment is obsolete: true
Attachment #8733019 - Flags: review?(honzab.moz) → review+
Comment on attachment 8733019 [details]
MozReview Request: Bug 1256473 - Cast values to avoid C4838 on VS2015; r?mayhemer

https://reviewboard.mozilla.org/r/41519/#review38183

::: netwerk/base/nsSocketTransport2.cpp:3028
(Diff revision 1)
>      // Windows allows idle time and retry interval to be set; NOT ping count.
>      struct tcp_keepalive keepalive_vals = {
> -        (int)aEnabled,
> +        (u_long)aEnabled,
>          // Windows uses msec.
> -        aIdleTime * 1000,
> -        aRetryInterval * 1000
> +        (u_long)(aIdleTime * 1000),
> +        (u_long)(aRetryInterval * 1000)

would u_long(aIdleTime) * 1000UL be better?  

it won't help much with overflows but will be IMO a bit more consistent at least regarding signing.
Comment on attachment 8733019 [details]
MozReview Request: Bug 1256473 - Cast values to avoid C4838 on VS2015; r?mayhemer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41519/diff/1-2/
https://reviewboard.mozilla.org/r/41519/#review38183

> would u_long(aIdleTime) * 1000UL be better?  
> 
> it won't help much with overflows but will be IMO a bit more consistent at least regarding signing.

Will land with the UL.
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/e8e0c5caadbc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.