|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
58 bytes, text/x-review-board-request
|Details | Review|
This warning gets converted to an error when building with Visual Studio 2015 Update 1 in automation.
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/
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/
help moving this forward plz
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
I would much prefer to fix the warning (for which I can't make sense) rather than disabling them all.
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.
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...
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.
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.
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/
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.
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!
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).
I'll reopen if this didn't fix it.