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)
Core
Networking
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.
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Attachment #8730420 -
Attachment is obsolete: true
Attachment #8730420 -
Flags: review?(odvarko)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8730439 -
Attachment is obsolete: true
Comment 4•8 years ago
|
||
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-
Assignee | ||
Updated•8 years ago
|
Attachment #8730441 -
Flags: review- → review?(honzab.moz)
Assignee | ||
Comment 5•8 years ago
|
||
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
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
The Try push reproduced the warning.
Comment 9•8 years ago
|
||
can you help move this forward mcmanus?
Comment 10•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8730441 -
Flags: review?(honzab.moz) → review+
Comment 11•8 years ago
|
||
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
Comment 12•8 years ago
|
||
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;
Updated•8 years ago
|
Assignee: nobody → gps
Whiteboard: [necko-active]
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6baf3d309647
Assignee | ||
Updated•8 years ago
|
Attachment #8730441 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41519/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41519/
Attachment #8733019 -
Flags: review?(honzab.moz)
Updated•8 years ago
|
Attachment #8733019 -
Flags: review?(honzab.moz) → review+
Comment 15•8 years ago
|
||
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.
Assignee | ||
Comment 16•8 years ago
|
||
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/
Assignee | ||
Comment 17•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e8e0c5caadbc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•