Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1494133003:
New macro: RTC_DEPRECATED (for annotating deprecated functions) (Closed)
|
Created:
5 years ago by kwiberg-webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNew macro: RTC_DEPRECATED (for annotating deprecated functions)
Committed: https://crrev.com/45fd9fe92c1cda7dbd9811fb807d9ae358017603
Cr-Commit-Position: refs/heads/master@{#11042}
Patch Set 1 : #Patch Set 2 : deprecate scoped_ptr::Pass instead, since it's more widely used #Patch Set 3 : final version, with no uses of the new macro #
Messages
Total messages: 24 (6 generated)
Patchset #1 (id:1) has been deleted
kwiberg@webrtc.org changed reviewers: + kjellander@webrtc.org, phoglund@webrtc.org
kwiberg@webrtc.org changed reviewers: + tommi@webrtc.org
Note: The uses of the macro in buffer.h and the changes to buffer_unittests.cc are just for illustration (for example, it's interesting to see which bots fail and which are ignoring the deprecation). I'll revert them before landing this.
On 2015/12/03 13:24:16, kwiberg-webrtc wrote: > Note: The uses of the macro in buffer.h and the changes to buffer_unittests.cc > are just for illustration (for example, it's interesting to see which bots fail > and which are ignoring the deprecation). I'll revert them before landing this. Looks promising. I guess we also want to see a run where it passes, so you'll need to add something like this to base_tests.gypi, right? cflags: [ '-Wno-deprecated-declarations', ]
On 2015/12/03 14:19:04, kjellander (webrtc) wrote: > On 2015/12/03 13:24:16, kwiberg-webrtc wrote: > > Note: The uses of the macro in buffer.h and the changes to buffer_unittests.cc > > are just for illustration (for example, it's interesting to see which bots > fail > > and which are ignoring the deprecation). I'll revert them before landing this. > > Looks promising. I guess we also want to see a run where it passes, so you'll > need to add something like this to base_tests.gypi, right? > cflags: [ > '-Wno-deprecated-declarations', > ] Actually, no; because we'd have to find the flag for MSVC as well (and other compilers too if we add them to the macro), and because that de-deprecates all deprecated functions in at least an entire compilation unit, I went with the renaming scheme documented in deprecation.h instead. (That was possible because we're only concerned with source and not binary compatibility.) The failures we see here are all because of uses of Buffer::Pass() outside of the unit tests.
Hmm. Do we compile without -Wdeprecated-declarations on the gn bots?
On 2015/12/03 14:30:38, kwiberg-webrtc wrote: > Hmm. Do we compile without -Wdeprecated-declarations on the gn bots? No they just don't have any test targets defined (yet).
On 2015/12/03 14:32:35, kjellander (webrtc) wrote: > On 2015/12/03 14:30:38, kwiberg-webrtc wrote: > > Hmm. Do we compile without -Wdeprecated-declarations on the gn bots? > > No they just don't have any test targets defined (yet). But they compile code where we use Buffer::Pass(). It's not only used in tests...
On 2015/12/03 14:30:38, kwiberg-webrtc wrote: > Hmm. Do we compile without -Wdeprecated-declarations on the gn bots? And no Windows bot is complaining... who knows about WebRTC on Windows?
On 2015/12/03 14:41:33, kwiberg-webrtc wrote: > On 2015/12/03 14:30:38, kwiberg-webrtc wrote: > > Hmm. Do we compile without -Wdeprecated-declarations on the gn bots? > > And no Windows bot is complaining... who knows about WebRTC on Windows? Tommi is our Windows guru, so let's wait for him to look at it.
On 2015/12/03 15:13:19, kjellander (webrtc) wrote: > On 2015/12/03 14:41:33, kwiberg-webrtc wrote: > > On 2015/12/03 14:30:38, kwiberg-webrtc wrote: > > > Hmm. Do we compile without -Wdeprecated-declarations on the gn bots? > > > > And no Windows bot is complaining... who knows about WebRTC on Windows? > > Tommi is our Windows guru, so let's wait for him to look at it. What's the warning level we compile with on Windows? (is it 4?) I wonder if _CRT_NONSTDC_NO_DEPRECATE or _WINSOCK_DEPRECATED_NO_WARNINGS could be teasing us. In any case, I can take a look tomorrow.
Removing myself (going on vacation)
On 2015/12/03 14:40:09, kwiberg-webrtc wrote: > On 2015/12/03 14:32:35, kjellander (webrtc) wrote: > > On 2015/12/03 14:30:38, kwiberg-webrtc wrote: > > > Hmm. Do we compile without -Wdeprecated-declarations on the gn bots? > > > > No they just don't have any test targets defined (yet). > > But they compile code where we use Buffer::Pass(). It's not only used in > tests... So I just tried deprecating scoped_ptr::Pass instead, and now the gn bots complain too. So you were right that the problem was simply that the gn bots weren't building anything that used the deprecated method. Tommi, did you get a chance to look into what's keeping the Widows bots from complaining?
On 2015/12/15 13:04:37, kwiberg-webrtc wrote: > Tommi, did you get a chance to look into what's keeping the Widows bots from > complaining? (Or alternatively, can we land this as is (minus the example use of RTC_DEPRECATED that causes the compile errors, of course), and just file a bug about the Windows issue? It should be plenty useful even without working Windows support.)
kwiberg@webrtc.org changed required reviewers: + kjellander@webrtc.org, tommi@webrtc.org
Please have a look at the final version, without the sample uses of the new macro.
lgtm
Nice work, thanks for helping out with this. LGTM
The CQ bit was checked by kwiberg@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1494133003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1494133003/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== New macro: RTC_DEPRECATED (for annotating deprecated functions) ========== to ========== New macro: RTC_DEPRECATED (for annotating deprecated functions) Committed: https://crrev.com/45fd9fe92c1cda7dbd9811fb807d9ae358017603 Cr-Commit-Position: refs/heads/master@{#11042} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/45fd9fe92c1cda7dbd9811fb807d9ae358017603 Cr-Commit-Position: refs/heads/master@{#11042} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
