Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(296)

Issue 2838713002: Simplify ALLOW_UNUSED_LOCAL to use (void)x directly instead of conditionally. (Closed)

Created:
3 years, 8 months ago by Kevin M
Modified:
3 years, 8 months ago
CC:
chromium-reviews, danakj+watch_chromium.org, vmpstr+watch_chromium.org, Wez
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Simplify ALLOW_UNUSED_LOCAL to use (void)x directly instead of conditionally. Add ALLOW_UNUSED_LOCAL implementation for path-sensitive analyzers. The current definition of ALLOW_UNUSED_LOCAL uses "(void)x" to suppress warnings that "x" was never used, but places the expression in a never-executed codepath of a ternary expression, forcing this statement to be a no-op. Static analyzers which are codepath sensitive, like Clang's scan-build, will only trace along the no-op codepath and therefore will never evaluate the voidification clause. The result is a lot of warning noise like this: "warning: Value stored to 'x' during its initialization is never read" This CL removes the ternary expression from ALLOW_UNUSED_LOCAL so that the voidification statement is evaluated by path sensitive checkers. The build size was not affected by this change, therefore it's reasonable to assume that this won't have an effect on runtime behavior. R=pkasting@chromium.org CC=wez@chromium.org BUG=687243 Review-Url: https://codereview.chromium.org/2838713002 Cr-Commit-Position: refs/heads/master@{#467215} Committed: https://chromium.googlesource.com/chromium/src/+/d10bb7f7a2f052d8240079b43572c9c5124a20c3

Patch Set 1 #

Total comments: 2

Patch Set 2 : Use pkasting's comment suggestion #

Total comments: 2

Patch Set 3 : Removed ternary expr & alternative impl for ALLOW_UNUSED_LOCAL #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -2 lines) Patch
M base/compiler_specific.h View 1 2 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 34 (14 generated)
Kevin M
3 years, 8 months ago (2017-04-24 20:16:05 UTC) #1
Peter Kasting
LGTM https://codereview.chromium.org/2838713002/diff/1/base/compiler_specific.h File base/compiler_specific.h (right): https://codereview.chromium.org/2838713002/diff/1/base/compiler_specific.h#newcode92 base/compiler_specific.h:92: #endif // defined(__clang_analyzer__) Nit: I think this would ...
3 years, 8 months ago (2017-04-24 21:59:25 UTC) #4
Kevin M
https://codereview.chromium.org/2838713002/diff/1/base/compiler_specific.h File base/compiler_specific.h (right): https://codereview.chromium.org/2838713002/diff/1/base/compiler_specific.h#newcode92 base/compiler_specific.h:92: #endif // defined(__clang_analyzer__) On 2017/04/24 21:59:25, Peter Kasting (catching ...
3 years, 8 months ago (2017-04-24 22:37:39 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2838713002/20001
3 years, 8 months ago (2017-04-24 22:38:37 UTC) #8
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/419157)
3 years, 8 months ago (2017-04-24 22:44:36 UTC) #10
Kevin M
thakis@ for OWNERS review
3 years, 8 months ago (2017-04-24 22:46:00 UTC) #12
Nico
https://codereview.chromium.org/2838713002/diff/20001/base/compiler_specific.h File base/compiler_specific.h (right): https://codereview.chromium.org/2838713002/diff/20001/base/compiler_specific.h#newcode91 base/compiler_specific.h:91: #define ALLOW_UNUSED_LOCAL(x) false ? (void)x : (void)0 What was ...
3 years, 8 months ago (2017-04-25 15:48:52 UTC) #13
Kevin Marshall
+danakj for alternative OWNER review
3 years, 8 months ago (2017-04-25 17:07:03 UTC) #15
Nico
I'm​ not sure if exchanging owners after the first one you picked asked a question ...
3 years, 8 months ago (2017-04-25 17:11:52 UTC) #16
Kevin Marshall
Oh, sorry about that - got a stale page in Chrome and didn't see the ...
3 years, 8 months ago (2017-04-25 17:17:31 UTC) #17
Nico
On Tue, Apr 25, 2017 at 1:17 PM, <marshallk@google.com> wrote: > Oh, sorry about that ...
3 years, 8 months ago (2017-04-25 18:36:32 UTC) #18
Peter Kasting
It's worth a shot. You'd want to make sure we don't generate worse code -- ...
3 years, 8 months ago (2017-04-25 18:55:20 UTC) #19
chromium-reviews
Will do a side by side comparison now. On Tue, Apr 25, 2017 at 11:55 ...
3 years, 8 months ago (2017-04-25 19:34:54 UTC) #20
Kevin M
The byte counts are identical for the current ternary expr #define and just using (void)x ...
3 years, 8 months ago (2017-04-25 20:00:48 UTC) #21
Nico
lgtm
3 years, 8 months ago (2017-04-25 20:06:43 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2838713002/40001
3 years, 8 months ago (2017-04-25 20:31:06 UTC) #26
Peter Kasting
Rad. Thanks for pushing on this, Nico, and doing the comparison, Kevin -- glad we ...
3 years, 8 months ago (2017-04-25 20:53:13 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/255052) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, ...
3 years, 8 months ago (2017-04-25 22:29:35 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2838713002/40001
3 years, 8 months ago (2017-04-26 01:38:54 UTC) #31
commit-bot: I haz the power
3 years, 8 months ago (2017-04-26 02:55:57 UTC) #34
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d10bb7f7a2f052d8240079b43572...

Powered by Google App Engine
This is Rietveld 408576698