dom\media\FileBlockCache.h(136): warning C4312: 'reinterpret_cast': conversion from 'int32_t' to 'void *' of greater size

RESOLVED FIXED in Firefox 48

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gps, Assigned: gerald)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
This warning gets converted to an error when building with Visual Studio 2015 Update 1 in automation.
(Reporter)

Comment 1

2 years ago
Created attachment 8730513 [details]
MozReview Request: Bug 1256533 - Disable C4312 to unblock compilation on VS2015; r?jya

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

2 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/
Component: Audio/Video → Audio/Video: Playback

Comment 3

2 years ago
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)
(Assignee)

Comment 7

2 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

2 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.
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+
(Assignee)

Comment 11

2 years ago
Created attachment 8732579 [details]
MozReview Request: Bug 1256533 - Use std::deque<int32_t> instead of nsDeque - r?cpearce

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

2 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 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

2 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 15

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/445a27517245

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/445a27517245
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Reporter)

Comment 17

2 years ago
I'll reopen if this didn't fix it.
Flags: needinfo?(gps)
(Assignee)

Updated

2 years ago
Blocks: 1259293
You need to log in before you can comment on or make changes to this bug.