Closed
Bug 1256750
Opened 8 years ago
Closed 8 years ago
media/webrtc/signaling/src/sdp/SipccSdpAttributeList.cpp(690): warning C4474: 'sscanf_s' : too many arguments passed for format string
Categories
(Core :: WebRTC: Signaling, defect, P2)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: gps, Unassigned)
References
Details
Attachments
(1 file, 2 obsolete files)
This warning is turned into an error when building with Visual Studio 2015 Update 1 in automation.
Reporter | ||
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, obviously. However, this warning occurs in 3rd party WebRTC code, so our hands are somewhat tied. There is precedence for allowing compiler warnings in other WebRTC projects. I guess signaling was just lucky in that it had none. Review commit: https://reviewboard.mozilla.org/r/40181/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40181/
Attachment #8730839 -
Flags: review?(rjesup)
Reporter | ||
Comment 2•8 years ago
|
||
Comment on attachment 8730839 [details] MozReview Request: Bug 1256750 - Disable C4474 to unblock compilation on VS2015; r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40181/diff/1-2/
Reporter | ||
Comment 3•8 years ago
|
||
This is similar to the previous patch except it applies to our C++ in tree as opposed to the GYP config. The same issue with the same header is encountered. Review commit: https://reviewboard.mozilla.org/r/40187/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40187/
Attachment #8730846 -
Flags: review?(rjesup)
Comment 4•8 years ago
|
||
the sizeof(unsigned*) is not needed for windows (that's only for strings and chars and arrays). See https://msdn.microsoft.com/en-us/library/t6z7bya3.aspx Byron, can we just fix this the right way?
Flags: needinfo?(docfaraday)
Comment 6•8 years ago
|
||
If we take it away we can simply make it use sscanf() on all platforms, or?
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40195/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40195/
Attachment #8730861 -
Flags: review?(rjesup)
Comment 8•8 years ago
|
||
(In reply to Nils Ohlmeier [:drno] from comment #6) > If we take it away we can simply make it use sscanf() on all platforms, or? Hmm, it seems that the way we're using it, there is no difference between the two functions. Probably?
Comment 9•8 years ago
|
||
There is one remaining difference actually; %x is used for windows, %xu for everything else, but I don't think the latter is actually what was intended. I'm pretty sure that scans for a hex number, followed by a 'u'.
Comment 10•8 years ago
|
||
Comment on attachment 8730861 [details] MozReview Request: Bug 1256750: Remove unnecessary sscanf_s parameter on windows, and fix format string everywhere else. r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40195/diff/1-2/
Attachment #8730861 -
Attachment description: MozReview Request: Bug 1256750: Remove unnecessary sscanf_s parameter on windows. r?jesup → MozReview Request: Bug 1256750: Remove unnecessary sscanf_s parameter on windows, and fix format string everywhere else. r?jesup
Updated•8 years ago
|
Attachment #8730861 -
Flags: review?(rjesup) → review+
Comment 11•8 years ago
|
||
Comment on attachment 8730861 [details] MozReview Request: Bug 1256750: Remove unnecessary sscanf_s parameter on windows, and fix format string everywhere else. r?jesup https://reviewboard.mozilla.org/r/40195/#review36725
Reporter | ||
Comment 12•8 years ago
|
||
Comment on attachment 8730839 [details] MozReview Request: Bug 1256750 - Disable C4474 to unblock compilation on VS2015; r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40181/diff/2-3/
Reporter | ||
Comment 13•8 years ago
|
||
Comment on attachment 8730846 [details] MozReview Request: Bug 1256750 - Disable C4474 to unblock compilation on VS2015; r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40187/diff/1-2/
Reporter | ||
Comment 14•8 years ago
|
||
Comment on attachment 8730839 [details] MozReview Request: Bug 1256750 - Disable C4474 to unblock compilation on VS2015; r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40181/diff/3-4/
Reporter | ||
Comment 15•8 years ago
|
||
Comment on attachment 8730846 [details] MozReview Request: Bug 1256750 - Disable C4474 to unblock compilation on VS2015; r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40187/diff/2-3/
Comment 16•8 years ago
|
||
gps: what's going on with these review updates? Is the r+'d patch to fix the root cause of the warning insufficient?
Flags: needinfo?(gps)
Reporter | ||
Comment 17•8 years ago
|
||
Comment on attachment 8730839 [details] MozReview Request: Bug 1256750 - Disable C4474 to unblock compilation on VS2015; r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40181/diff/4-5/
Reporter | ||
Comment 18•8 years ago
|
||
Comment on attachment 8730846 [details] MozReview Request: Bug 1256750 - Disable C4474 to unblock compilation on VS2015; r?jesup Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40187/diff/3-4/
Reporter | ||
Comment 19•8 years ago
|
||
I keep finding the same warning from the header creep into additional compilation units (like C++ tests). Please look at the latest versions of the diffs.
Flags: needinfo?(gps)
Comment 20•8 years ago
|
||
gps: can you please provide us with the locations of your new findings so we can try to fix them rather disabling the compiler warning in lots of places? Note: I doubt that most of us have access to VS2015 so we can't easily locate them our self.
Flags: needinfo?(gps)
Reporter | ||
Comment 21•8 years ago
|
||
I'm pretty sure it is all the same warning that is in the bug summary - we just get it from compiling a number of different things (like the tests). Here are links to some try failures: https://treeherder.mozilla.org/logviewer.html#?job_id=18071349&repo=try https://treeherder.mozilla.org/logviewer.html#?job_id=18064736&repo=try
Flags: needinfo?(gps)
Comment 22•8 years ago
|
||
If it's from the same file/line, this fix should get them all. The second one is something entirely different. We'd far rather get a list of all the failures like this and fix them than a blanket disable of warnings/wanrings-as-errors
Updated•8 years ago
|
Rank: 23
Priority: -- → P2
Reporter | ||
Comment 23•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e4286a4a17f
Reporter | ||
Comment 24•8 years ago
|
||
Hopefully this Try push won't insta fail: https://treeherder.mozilla.org/#/jobs?repo=try&revision=adaa17566ea1
Reporter | ||
Comment 25•8 years ago
|
||
Comment on attachment 8730861 [details] MozReview Request: Bug 1256750: Remove unnecessary sscanf_s parameter on windows, and fix format string everywhere else. r?jesup The try push for this looks good (ignore the failure on 64-bit - I'm 95% confident I saw this failure on 32-bit builds and those were green). I'll withdrawal my review requests.
Attachment #8730861 -
Flags: feedback+
Reporter | ||
Updated•8 years ago
|
Attachment #8730839 -
Attachment is obsolete: true
Attachment #8730839 -
Flags: review?(rjesup)
Reporter | ||
Updated•8 years ago
|
Attachment #8730846 -
Attachment is obsolete: true
Attachment #8730846 -
Flags: review?(rjesup)
Comment 29•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6e2f198c3cfd
Status: NEW → 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
•