Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1510233002:
clang/win: Fix -Wextra warnings in webrtc. (Closed)
Descriptionclang/win: Fix -Wextra warnings in webrtc.
Fixes one sign mismatch warning, and one "const has no effect and is
ignored" warning.
BUG=chromium:567877
Committed: https://crrev.com/61a90f94b60979a1f5569ea71739e233455e296f
Cr-Commit-Position: refs/heads/master@{#10976}
Patch Set 1 #Patch Set 2 : onemore #Patch Set 3 : debug #
Total comments: 2
Patch Set 4 : comments #
Total comments: 3
Patch Set 5 : backto1 #
Messages
Total messages: 26 (10 generated)
thakis@chromium.org changed reviewers: + sergeyu@chromium.org
thakis@chromium.org changed reviewers: + perkj@chromium.org
+perkj too in case he's in a better timezone than sergeyu (who might not be in yet)
lgtm, but please see my suggestion https://codereview.webrtc.org/1510233002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc (right): https://codereview.webrtc.org/1510233002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:239: assert(tls_index_.Value() != static_cast<int32_t>(TLS_OUT_OF_INDEXES)); maybe define kTlsIndexNotSet instead of using TLS_OUT_OF_INDEXES everywhere in this file? TLS_OUT_OF_INDEXES is useful only when validating result of TlsAlloc() and this code doesn't actually do that (see line 376). It asserts that TlsAlloc() always returns something other than TLS_OUT_OF_INDEXES.
https://codereview.webrtc.org/1510233002/diff/40001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc (right): https://codereview.webrtc.org/1510233002/diff/40001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:239: assert(tls_index_.Value() != static_cast<int32_t>(TLS_OUT_OF_INDEXES)); done
The CQ bit was checked by thakis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.webrtc.org/1510233002/#ps60001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1510233002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510233002/60001
https://codereview.webrtc.org/1510233002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc (right): https://codereview.webrtc.org/1510233002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:36: int32_t kTlsIndexNotSet = 0; // Should not be a valid pointer. Not sure we can use 0 here. Can TlsAlloc() return 0?
The CQ bit was unchecked by thakis@chromium.org
https://codereview.webrtc.org/1510233002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc (right): https://codereview.webrtc.org/1510233002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:36: int32_t kTlsIndexNotSet = 0; // Should not be a valid pointer. On 2015/12/09 22:31:51, Sergey Ulanov wrote: > Not sure we can use 0 here. Can TlsAlloc() return 0? Hm, you're right. Reverted this to patch set 1.
The CQ bit was checked by thakis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.webrtc.org/1510233002/#ps80001 (title: "backto1")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1510233002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510233002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by thakis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1510233002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1510233002/80001
https://codereview.webrtc.org/1510233002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc (right): https://codereview.webrtc.org/1510233002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:36: int32_t kTlsIndexNotSet = 0; // Should not be a valid pointer. On 2015/12/10 15:05:46, Nico wrote: > On 2015/12/09 22:31:51, Sergey Ulanov wrote: > > Not sure we can use 0 here. Can TlsAlloc() return 0? > > Hm, you're right. Reverted this to patch set 1. Sorry for not being clear, what I was suggesting initially is to define kTlsIndexNotSet = static_cast<int32_t>(TLS_OUT_OF_INDEXES), which is the same as -1.
On 2015/12/10 17:29:55, Sergey Ulanov wrote: > https://codereview.webrtc.org/1510233002/diff/60001/webrtc/modules/desktop_ca... > File webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc > (right): > > https://codereview.webrtc.org/1510233002/diff/60001/webrtc/modules/desktop_ca... > webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:36: int32_t > kTlsIndexNotSet = 0; // Should not be a valid pointer. > On 2015/12/10 15:05:46, Nico wrote: > > On 2015/12/09 22:31:51, Sergey Ulanov wrote: > > > Not sure we can use 0 here. Can TlsAlloc() return 0? > > > > Hm, you're right. Reverted this to patch set 1. > > Sorry for not being clear, what I was suggesting initially is to define > kTlsIndexNotSet = static_cast<int32_t>(TLS_OUT_OF_INDEXES), which is the same as > -1. That removes three static_casts at the cost of one layer of indirection. Neither seems obviously better than the other imho, so I figured I'd go with the version touching fewer lines :-)
...also, do you know what's up with the _baremetal bots not kicking off any builds? Do I need to NOTRY=true this?
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== clang/win: Fix -Wextra warnings in webrtc. Fixes one sign mismatch warning, and one "const has no effect and is ignored" warning. BUG=chromium:567877 ========== to ========== clang/win: Fix -Wextra warnings in webrtc. Fixes one sign mismatch warning, and one "const has no effect and is ignored" warning. BUG=chromium:567877 Committed: https://crrev.com/61a90f94b60979a1f5569ea71739e233455e296f Cr-Commit-Position: refs/heads/master@{#10976} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/61a90f94b60979a1f5569ea71739e233455e296f Cr-Commit-Position: refs/heads/master@{#10976}
Message was sent while issue was closed.
On 2015/12/10 17:34:49, Nico wrote: > ...also, do you know what's up with the _baremetal bots not kicking off any > builds? Do I need to NOTRY=true this? This was caused by them having too long queues, causing the jobs to time out. We're running over the limit of our capacity at peak hours, which is unfortunate. I haven't been able to spend enough time addressing those problems (getting analyze to work is my best bet: https://code.google.com/p/chromium/issues/detail?id=482463). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
