|
|
Created:
4 years, 1 month ago by kjellander_webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, zhengzhonghou_agora.io, tterriberry_mozilla.com, sdk-team_agora.io, the sun, perkj_webrtc, mflodman Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionWebRTC: Fix and enable -Woverloaded-virtual warnings.
Essentially applying the same change as in
https://codereview.webrtc.org/2023413002 in more locations.
There's only one change affecting production code: enabling the warning
for webrtc/media:rtc_media. The rest are test changes.
With these changes, the only place the warning is disabled is in the Windows
implementation of webrtc/modules/video_capture:video_capture_internal_impl,
which is harder to fix, since it relies on sample code from the Windows SDK.
BUG=webrtc:6653
NOTRY=True
Committed: https://crrev.com/71a1b61c4f1b9afe769e4b0d448008aa3fa322e4
Cr-Commit-Position: refs/heads/master@{#14938}
Patch Set 1 #Patch Set 2 : Fixed errors and reverted video capture changes #
Messages
Total messages: 29 (15 generated)
Description was changed from ========== WebRTC: Enable -Woverloaded-virtual everywhere BUG=webrtc:6653 ========== to ========== WebRTC: Fix -Woverloaded-virtual warnings. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 ==========
Description was changed from ========== WebRTC: Fix -Woverloaded-virtual warnings. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 ========== to ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 ==========
Description was changed from ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 ========== to ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. Essentially applying the same change as in https://codereview.webrtc.org/2023413002 in more locations. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 ==========
The CQ bit was checked by kjellander@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. Essentially applying the same change as in https://codereview.webrtc.org/2023413002 in more locations. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 ========== to ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. Essentially applying the same change as in https://codereview.webrtc.org/2023413002 in more locations. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 ==========
Description was changed from ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. Essentially applying the same change as in https://codereview.webrtc.org/2023413002 in more locations. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 ========== to ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. Essentially applying the same change as in https://codereview.webrtc.org/2023413002 in more locations. There's only one change affecting production code: enabling the warning for webrtc/media:rtc_media. The rest are test changes. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 ==========
kjellander@webrtc.org changed reviewers: + phoglund@webrtc.org
This doesn't change any of the production code though, but it enables the warning for rtc_media. Do you know if this is sufficient for the recent internal breakage?
The CQ bit was unchecked by kjellander@webrtc.org
It's not going to fix anything, since the error was with peerconnection_jni.cc. I guess there was a separate CL to land a fix?
I don't see how this prevents the error. The error was with peerconnection_jni.cc which is in the libjingle_peerconnection_jni target, and you're not modifying the flag set for that target as far I can see (just unit test targets). Or am I missing something?
On 2016/11/03 10:28:42, phoglund wrote: > I don't see how this prevents the error. The error was with > peerconnection_jni.cc which is in the libjingle_peerconnection_jni target, and > you're not modifying the flag set for that target as far I can see (just unit > test targets). Or am I missing something? Hm. I read this as "-Woverloaded-virtual is set for all targets if we're posix but not android": https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?q=common_con...
On 2016/11/03 10:24:37, phoglund wrote: > It's not going to fix anything, since the error was with peerconnection_jni.cc. > I guess there was a separate CL to land a fix? I created this CL before I learned that the error was introduced by https://codereview.webrtc.org/2449993003/ (and fixed in https://codereview.webrtc.org/2468253002/). Since this CL enabled the warning for rtc_media, it should have caught the error in our own build. I think the peerconnection_jni.cc error you're referring to is the one that was fixed by deadbeef@ in https://codereview.webrtc.org/2023413002.
On 2016/11/03 10:36:05, phoglund wrote: > On 2016/11/03 10:28:42, phoglund wrote: > > I don't see how this prevents the error. The error was with > > peerconnection_jni.cc which is in the libjingle_peerconnection_jni target, and > > you're not modifying the flag set for that target as far I can see (just unit > > test targets). Or am I missing something? > > Hm. I read this as "-Woverloaded-virtual is set for all targets if we're posix > but not android": > https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?q=common_con... For GYP, it's set for all Posix (which includes Android): https://cs.chromium.org/chromium/src/third_party/webrtc/build/common.gypi?rcl... For GN, it's similarly set, but is incorrectly excluding Android ARM64 builds. That's a bug that I can fix in a separate CL. It is set for the other Android configs though (notice the || ).
On 2016/11/03 10:58:15, kjellander_webrtc wrote: > On 2016/11/03 10:36:05, phoglund wrote: > > On 2016/11/03 10:28:42, phoglund wrote: > > > I don't see how this prevents the error. The error was with > > > peerconnection_jni.cc which is in the libjingle_peerconnection_jni target, > and > > > you're not modifying the flag set for that target as far I can see (just > unit > > > test targets). Or am I missing something? > > > > Hm. I read this as "-Woverloaded-virtual is set for all targets if we're posix > > but not android": > > > https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?q=common_con... > > For GYP, it's set for all Posix (which includes Android): > https://cs.chromium.org/chromium/src/third_party/webrtc/build/common.gypi?rcl... > For GN, it's similarly set, but is incorrectly excluding Android ARM64 builds. > That's a bug that I can fix in a separate CL. It is set for the other Android > configs though (notice the || ). Ah, I see. Ok, so there are two errors we've seen, one in media/.../webrtcvideoengine2.cc and one in peerconnection_jni.cc. This CL will enable warnings for the former but not the latter. That sound right?
On 2016/11/03 11:59:34, phoglund wrote: > On 2016/11/03 10:58:15, kjellander_webrtc wrote: > > On 2016/11/03 10:36:05, phoglund wrote: > > > On 2016/11/03 10:28:42, phoglund wrote: > > > > I don't see how this prevents the error. The error was with > > > > peerconnection_jni.cc which is in the libjingle_peerconnection_jni target, > > and > > > > you're not modifying the flag set for that target as far I can see (just > > unit > > > > test targets). Or am I missing something? > > > > > > Hm. I read this as "-Woverloaded-virtual is set for all targets if we're > posix > > > but not android": > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?q=common_con... > > > > For GYP, it's set for all Posix (which includes Android): > > > https://cs.chromium.org/chromium/src/third_party/webrtc/build/common.gypi?rcl... > > For GN, it's similarly set, but is incorrectly excluding Android ARM64 builds. > > That's a bug that I can fix in a separate CL. It is set for the other Android > > configs though (notice the || ). > > Ah, I see. > > Ok, so there are two errors we've seen, one in media/.../webrtcvideoengine2.cc > and one in peerconnection_jni.cc. This CL will enable warnings for the former > but not the latter. That sound right? Yes, the latter already got warnings enabled shortly after the fix was submitted: https://codereview.webrtc.org/2035593003/ (peerconnection_jni.cc is in that GYP target) So, do I get approval for this, since it's at least a small improvement?
On 2016/11/03 12:26:15, kjellander_webrtc wrote: > On 2016/11/03 11:59:34, phoglund wrote: > > On 2016/11/03 10:58:15, kjellander_webrtc wrote: > > > On 2016/11/03 10:36:05, phoglund wrote: > > > > On 2016/11/03 10:28:42, phoglund wrote: > > > > > I don't see how this prevents the error. The error was with > > > > > peerconnection_jni.cc which is in the libjingle_peerconnection_jni > target, > > > and > > > > > you're not modifying the flag set for that target as far I can see (just > > > unit > > > > > test targets). Or am I missing something? > > > > > > > > Hm. I read this as "-Woverloaded-virtual is set for all targets if we're > > posix > > > > but not android": > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/BUILD.gn?q=common_con... > > > > > > For GYP, it's set for all Posix (which includes Android): > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/build/common.gypi?rcl... > > > For GN, it's similarly set, but is incorrectly excluding Android ARM64 > builds. > > > That's a bug that I can fix in a separate CL. It is set for the other > Android > > > configs though (notice the || ). > > > > Ah, I see. > > > > Ok, so there are two errors we've seen, one in media/.../webrtcvideoengine2.cc > > and one in peerconnection_jni.cc. This CL will enable warnings for the former > > but not the latter. That sound right? > > Yes, the latter already got warnings enabled shortly after the fix was > submitted: https://codereview.webrtc.org/2035593003/ (peerconnection_jni.cc is > in that GYP target) > So, do I get approval for this, since it's at least a small improvement? Ohh, so the peerconnection error isn't one of the current ones, but the old error? Then I understand. lgtm
kjellander@webrtc.org changed reviewers: + deadbeef@webrtc.org
+deadbeef@ for webrtc/api (and since added TODO's are copies of his).
lgtm. Now I really want to get rid of those duplicate methods though.
Description was changed from ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. Essentially applying the same change as in https://codereview.webrtc.org/2023413002 in more locations. There's only one change affecting production code: enabling the warning for webrtc/media:rtc_media. The rest are test changes. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 ========== to ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. Essentially applying the same change as in https://codereview.webrtc.org/2023413002 in more locations. There's only one change affecting production code: enabling the warning for webrtc/media:rtc_media. The rest are test changes. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 NOTRY=True ==========
Description was changed from ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. Essentially applying the same change as in https://codereview.webrtc.org/2023413002 in more locations. There's only one change affecting production code: enabling the warning for webrtc/media:rtc_media. The rest are test changes. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 ========== to ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. Essentially applying the same change as in https://codereview.webrtc.org/2023413002 in more locations. There's only one change affecting production code: enabling the warning for webrtc/media:rtc_media. The rest are test changes. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 NOTRY=True ==========
The CQ bit was checked by kjellander@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. Essentially applying the same change as in https://codereview.webrtc.org/2023413002 in more locations. There's only one change affecting production code: enabling the warning for webrtc/media:rtc_media. The rest are test changes. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 NOTRY=True ========== to ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. Essentially applying the same change as in https://codereview.webrtc.org/2023413002 in more locations. There's only one change affecting production code: enabling the warning for webrtc/media:rtc_media. The rest are test changes. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. Essentially applying the same change as in https://codereview.webrtc.org/2023413002 in more locations. There's only one change affecting production code: enabling the warning for webrtc/media:rtc_media. The rest are test changes. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 NOTRY=True ========== to ========== WebRTC: Fix and enable -Woverloaded-virtual warnings. Essentially applying the same change as in https://codereview.webrtc.org/2023413002 in more locations. There's only one change affecting production code: enabling the warning for webrtc/media:rtc_media. The rest are test changes. With these changes, the only place the warning is disabled is in the Windows implementation of webrtc/modules/video_capture:video_capture_internal_impl, which is harder to fix, since it relies on sample code from the Windows SDK. BUG=webrtc:6653 NOTRY=True Committed: https://crrev.com/71a1b61c4f1b9afe769e4b0d448008aa3fa322e4 Cr-Commit-Position: refs/heads/master@{#14938} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/71a1b61c4f1b9afe769e4b0d448008aa3fa322e4 Cr-Commit-Position: refs/heads/master@{#14938} |