Closed
Bug 1256533
Opened 8 years ago
Closed 8 years ago
dom\media\FileBlockCache.h(136): warning C4312: 'reinterpret_cast': conversion from 'int32_t' to 'void *' of greater size
Categories
(Core :: Audio/Video: Playback, defect)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: gps, Assigned: mozbugz)
References
Details
Attachments
(1 file, 1 obsolete file)
This warning gets converted to 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. So hopefully this patch never lands because someone insists on fixing the underlying problem instead. But if it does land, hopefully the workaround is only temporary. Review commit: https://reviewboard.mozilla.org/r/39963/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39963/
Attachment #8730513 -
Flags: review?(jyavenard)
Reporter | ||
Comment 2•8 years ago
|
||
Comment on attachment 8730513 [details] MozReview Request: Bug 1256533 - Disable C4312 to unblock compilation on VS2015; r?jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39963/diff/1-2/
Updated•8 years ago
|
Component: Audio/Video → Audio/Video: Playback
Comment 4•8 years ago
|
||
Gerald, can you take this over ? you know this just as much as I do, and your C++fu is much better than mine. thank you
Flags: needinfo?(gsquelart)
Comment 5•8 years ago
|
||
I would much prefer to fix the warning (for which I can't make sense) rather than disabling them all.
Updated•8 years ago
|
Flags: needinfo?(mreavy)
Attachment #8730513 -
Flags: review?(jyavenard) → review?(cpearce)
Comment 6•8 years ago
|
||
Chris -- After briefly chatting with jya in irc, I've moved the review to you. jya doesn't feel comfortable reviewing, and IMO although these patches are simple, gps is asking permission to disable warnings -- which is really a module owner's call. The choice is to disable or fix the warnings. In webrtc we're fixing them instead of disabling. It's your call which you prefer to do here. Thanks.
Flags: needinfo?(cpearce)
Assignee | ||
Comment 7•8 years ago
|
||
At a glance, the code is safe, so disabling the warning would not hide an actual issue -- for now. I'll see if I can work out a better fix in the next couple of days, unless Chris interjects in the meantime...
Flags: needinfo?(gsquelart)
Reporter | ||
Comment 8•8 years ago
|
||
I would prefer the warning just got fixed. I only submitted the patches to disable the warnings to give people (including me) an easy way out so we could unblock switching to VS2015. Bug 1124033 comment #21 contains instructions for testing warnings fixes on Try with VS2015. If your build job lasts longer than 1hr, you've fixed the warning. If you submit a patch that tries to fix the warning, needinfo me and I can push it for you.
Comment 9•8 years ago
|
||
We should use std::queue<int32_t> instead of nsDeque. I'm happy for Gerald to handle that. The only reason we use nsDeque is at the time I wrote this code we weren't allowed to use std::anything.
Flags: needinfo?(cpearce)
Comment 10•8 years ago
|
||
Comment on attachment 8730513 [details] MozReview Request: Bug 1256533 - Disable C4312 to unblock compilation on VS2015; r?jya https://reviewboard.mozilla.org/r/39963/#review37525 Am fine with disabling warning until such a time as we fix the acutal warning. Would prefer to fix the code to not generate the warning.
Attachment #8730513 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 11•8 years ago
|
||
The queue of pending block indexes was implemented using nsDeque where item pointers were perverted into pure 32-bit numbers, causing a size mismatch on 64-bit platforms, which was picked by VS2015. which would be unreasonably-difficult with a pure queue. Review commit: https://reviewboard.mozilla.org/r/41231/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41231/
Attachment #8732579 -
Flags: review?(cpearce)
Assignee | ||
Comment 12•8 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e92bb50dbe9 Gregory, could you please try with VS2015 it if you can? (I'm uncertain how to do your magic trick from comment 8; I also tried on my local VS2015 install, but couldn't even get the original warning.) I'm confident the warning is gone as there is no pointer<->number casts anymore. But it's always possible other ones could have crept in with the replacement code.
Assignee: nobody → gsquelart
Flags: needinfo?(gps)
Comment 13•8 years ago
|
||
Comment on attachment 8732579 [details] MozReview Request: Bug 1256533 - Use std::deque<int32_t> instead of nsDeque - r?cpearce https://reviewboard.mozilla.org/r/41231/#review37793 Thanks!
Attachment #8732579 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8730513 [details] MozReview Request: Bug 1256533 - Disable C4312 to unblock compilation on VS2015; r?jya As the actual fix has been accepted, I'm obsoleting this patch (disabling the warning).
Attachment #8730513 -
Attachment is obsolete: true
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/445a27517245
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
•