|
|
Created:
4 years, 1 month ago by kwiberg-webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImprove RTC_DCHECK_op so that it won't trigger useless compiler warnings
Before this change, with DCHECKs switched off, this sort of check
size_t index = ...;
RTC_DCHECK_GE(index, 0u);
would cause GCC (but no other compiler that we use) to complain
that unsigned values are always greater than or equal to 0. With
this change, it no longer complains.
(It was and remains the case that there was no complaints if
DCHECKs were switched on, or if you used RTC_CHECK_op.)
The reason for doing this change is that it isn't useful for the
compiler to force us to remove DCHECKs just because their
condition can be verified statically. That causes us to remove
the checks, and once that's happened, future code changes are free
to violate the removed checks and no one will know...
BUG=webrtc:6620
Committed: https://crrev.com/32bcaf61f52f604c7e268533b43f1ee0a61dff43
Cr-Commit-Position: refs/heads/master@{#14805}
Patch Set 1 #
Total comments: 2
Patch Set 2 : review comment #Messages
Total messages: 22 (10 generated)
Description was changed from ========== Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings Before this change, with DCHECKs switched off, this sort of check size_t index = ...; RTC_DCHECK_GE(index, 0u); would cause GCC to complain that unsigned values are always greater than or equal to 0. With this change, it no longer complains. (It was and remains the case that there was no complaints if DCHECKs were switched on, or if you used RTC_CHECK_op.) ========== to ========== Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings Before this change, with DCHECKs switched off, this sort of check size_t index = ...; RTC_DCHECK_GE(index, 0u); would cause GCC (but no other compiler that we use) to complain that unsigned values are always greater than or equal to 0. With this change, it no longer complains. (It was and remains the case that there was no complaints if DCHECKs were switched on, or if you used RTC_CHECK_op.) ==========
kwiberg@webrtc.org changed reviewers: + kjellander@webrtc.org, ossu@webrtc.org
Because I don't think it's useful for the compiler to force us to remove DCHECKs just because their condition can be verified statically. That causes us to remove the check, and once that's happened, future code changes are free to violate the removed check and no one will know... I'm also considering a follow-up that would make it so that you don't have to write a "u" postfix to explicitly make an unsigned constant when comparing with an unsigned variable...
I tried looking at the code and I'm not sure why this doesn't happen with CHECKs, but I'm glad it doesn't. If this change stops us getting an error with DCHECKs, then it LGTM. I added a comment as well. https://codereview.webrtc.org/2455943002/diff/1/webrtc/base/checks.h File webrtc/base/checks.h (right): https://codereview.webrtc.org/2455943002/diff/1/webrtc/base/checks.h#newcode102 webrtc/base/checks.h:102: (void)(a), (void)(b), true)) Why ", true" at the end? Would you end up with a dual (void) (void) cast in RTC_EAT_STREAM_PARAMETERS and that makes the compiler uncomfortable?
Description was changed from ========== Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings Before this change, with DCHECKs switched off, this sort of check size_t index = ...; RTC_DCHECK_GE(index, 0u); would cause GCC (but no other compiler that we use) to complain that unsigned values are always greater than or equal to 0. With this change, it no longer complains. (It was and remains the case that there was no complaints if DCHECKs were switched on, or if you used RTC_CHECK_op.) ========== to ========== Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings Before this change, with DCHECKs switched off, this sort of check size_t index = ...; RTC_DCHECK_GE(index, 0u); would cause GCC (but no other compiler that we use) to complain that unsigned values are always greater than or equal to 0. With this change, it no longer complains. (It was and remains the case that there was no complaints if DCHECKs were switched on, or if you used RTC_CHECK_op.) The reason for doing this change is that it isn't useful for the compiler to force us to remove DCHECKs just because their condition can be verified statically. That causes us to remove the checks, and once that's happened, future code changes are free to violate the removed checks and no one will know... ==========
On 2016/10/27 13:11:01, ossu wrote: > I tried looking at the code and I'm not sure why this doesn't happen with > CHECKs, but I'm glad it doesn't. It's because in that case, we end up sending the rhs and lhs as separate arguments to a function, and the compiler isn't smart enough to follow the values through all the hoops. (Or maybe it chooses not to, on purpose, because it would cause too many false positives.)
https://codereview.webrtc.org/2455943002/diff/1/webrtc/base/checks.h File webrtc/base/checks.h (right): https://codereview.webrtc.org/2455943002/diff/1/webrtc/base/checks.h#newcode102 webrtc/base/checks.h:102: (void)(a), (void)(b), true)) On 2016/10/27 13:11:01, ossu wrote: > Why ", true" at the end? Would you end up with a dual (void) (void) cast in > RTC_EAT_STREAM_PARAMETERS and that makes the compiler uncomfortable? Hmm. I wrote this before I changed RTC_EAT_STREAM_PARAMETERS (see above), so I needed it to be convertible to bool, which wasn't the case for all types that b has in practice. But I should be able to change that now, I guess.
On 2016/10/27 15:17:30, kwiberg-webrtc wrote: > On 2016/10/27 13:11:01, ossu wrote: > > I tried looking at the code and I'm not sure why this doesn't happen with > > CHECKs, but I'm glad it doesn't. > > It's because in that case, we end up sending the rhs and lhs as separate > arguments to a function, and the compiler isn't smart enough to follow the > values through all the hoops. (Or maybe it chooses not to, on purpose, because > it would cause too many false positives.) Ah, alright, that makes sense!
I know to little about macros to provide useful input here. I trust ossu! Thanks for dealing with this. I guess I will discard https://codereview.webrtc.org/2450223002/ once this is landed, if this fixes the Android build when dcheck_always_on=false?
The CQ bit was checked by kwiberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org Link to the patchset: https://codereview.webrtc.org/2455943002/#ps20001 (title: "review comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9614)
On 2016/10/27 19:56:39, kjellander_webrtc wrote: > I guess I will discard https://codereview.webrtc.org/2450223002/ once this is > landed, if this fixes the Android build when dcheck_always_on=false? Yes. I tested it locally with gn gen out/no-dcheck --args='use_goma=true is_debug=false dcheck_always_on=false target_os="android" symbol_level=1' This build failed before this CL, and succeeds afterwards.
Description was changed from ========== Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings Before this change, with DCHECKs switched off, this sort of check size_t index = ...; RTC_DCHECK_GE(index, 0u); would cause GCC (but no other compiler that we use) to complain that unsigned values are always greater than or equal to 0. With this change, it no longer complains. (It was and remains the case that there was no complaints if DCHECKs were switched on, or if you used RTC_CHECK_op.) The reason for doing this change is that it isn't useful for the compiler to force us to remove DCHECKs just because their condition can be verified statically. That causes us to remove the checks, and once that's happened, future code changes are free to violate the removed checks and no one will know... ========== to ========== Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings Before this change, with DCHECKs switched off, this sort of check size_t index = ...; RTC_DCHECK_GE(index, 0u); would cause GCC (but no other compiler that we use) to complain that unsigned values are always greater than or equal to 0. With this change, it no longer complains. (It was and remains the case that there was no complaints if DCHECKs were switched on, or if you used RTC_CHECK_op.) The reason for doing this change is that it isn't useful for the compiler to force us to remove DCHECKs just because their condition can be verified statically. That causes us to remove the checks, and once that's happened, future code changes are free to violate the removed checks and no one will know... BUG=webrtc:6620 ==========
The CQ bit was checked by kwiberg@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 ========== Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings Before this change, with DCHECKs switched off, this sort of check size_t index = ...; RTC_DCHECK_GE(index, 0u); would cause GCC (but no other compiler that we use) to complain that unsigned values are always greater than or equal to 0. With this change, it no longer complains. (It was and remains the case that there was no complaints if DCHECKs were switched on, or if you used RTC_CHECK_op.) The reason for doing this change is that it isn't useful for the compiler to force us to remove DCHECKs just because their condition can be verified statically. That causes us to remove the checks, and once that's happened, future code changes are free to violate the removed checks and no one will know... BUG=webrtc:6620 ========== to ========== Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings Before this change, with DCHECKs switched off, this sort of check size_t index = ...; RTC_DCHECK_GE(index, 0u); would cause GCC (but no other compiler that we use) to complain that unsigned values are always greater than or equal to 0. With this change, it no longer complains. (It was and remains the case that there was no complaints if DCHECKs were switched on, or if you used RTC_CHECK_op.) The reason for doing this change is that it isn't useful for the compiler to force us to remove DCHECKs just because their condition can be verified statically. That causes us to remove the checks, and once that's happened, future code changes are free to violate the removed checks and no one will know... BUG=webrtc:6620 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings Before this change, with DCHECKs switched off, this sort of check size_t index = ...; RTC_DCHECK_GE(index, 0u); would cause GCC (but no other compiler that we use) to complain that unsigned values are always greater than or equal to 0. With this change, it no longer complains. (It was and remains the case that there was no complaints if DCHECKs were switched on, or if you used RTC_CHECK_op.) The reason for doing this change is that it isn't useful for the compiler to force us to remove DCHECKs just because their condition can be verified statically. That causes us to remove the checks, and once that's happened, future code changes are free to violate the removed checks and no one will know... BUG=webrtc:6620 ========== to ========== Improve RTC_DCHECK_op so that it won't trigger useless compiler warnings Before this change, with DCHECKs switched off, this sort of check size_t index = ...; RTC_DCHECK_GE(index, 0u); would cause GCC (but no other compiler that we use) to complain that unsigned values are always greater than or equal to 0. With this change, it no longer complains. (It was and remains the case that there was no complaints if DCHECKs were switched on, or if you used RTC_CHECK_op.) The reason for doing this change is that it isn't useful for the compiler to force us to remove DCHECKs just because their condition can be verified statically. That causes us to remove the checks, and once that's happened, future code changes are free to violate the removed checks and no one will know... BUG=webrtc:6620 Committed: https://crrev.com/32bcaf61f52f604c7e268533b43f1ee0a61dff43 Cr-Commit-Position: refs/heads/master@{#14805} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/32bcaf61f52f604c7e268533b43f1ee0a61dff43 Cr-Commit-Position: refs/heads/master@{#14805} |