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)

defect
Not set
normal

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.
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)
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/
Component: Audio/Video → Audio/Video: Playback
help moving this forward plz
Flags: needinfo?(mreavy)
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)
I would much prefer to fix the warning (for which I can't make sense) rather than disabling them all.
Flags: needinfo?(mreavy)
Attachment #8730513 - Flags: review?(jyavenard) → review?(cpearce)
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)
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)
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.
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 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+
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)
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/445a27517245
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
I'll reopen if this didn't fix it.
Flags: needinfo?(gps)
You need to log in before you can comment on or make changes to this bug.