|
|
DescriptionSimplify 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 #Messages
Total messages: 34 (14 generated)
The CQ bit was checked by kmarshall@chromium.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.chromium.or...
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#ne... base/compiler_specific.h:92: #endif // defined(__clang_analyzer__) Nit: I think this would be easier to read with comment above the #if: // The common-case implementation references |x| in such a way that it's never // evaluated. Static analysis builds still see this as not referencing |x|, so // in such cases we reference it directly to make the compiler happy, at the // risk of unintended side effects. #if defined(__clang_analyzer__) #define ALLOW_UNUSED_LOCAL(x) (void)x #else #define ALLOW_UNUSED_LOCAL(x) false ? (void)x : (void)0 #endif
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#ne... base/compiler_specific.h:92: #endif // defined(__clang_analyzer__) On 2017/04/24 21:59:25, Peter Kasting (catching up) wrote: > Nit: I think this would be easier to read with comment above the #if: > > // The common-case implementation references |x| in such a way that it's never > // evaluated. Static analysis builds still see this as not referencing |x|, so > // in such cases we reference it directly to make the compiler happy, at the > // risk of unintended side effects. > #if defined(__clang_analyzer__) > #define ALLOW_UNUSED_LOCAL(x) (void)x > #else > #define ALLOW_UNUSED_LOCAL(x) false ? (void)x : (void)0 > #endif Yes, that's an improvement. Thanks, done.
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2838713002/#ps20001 (title: "Use pkasting's comment suggestion")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
kmarshall@chromium.org changed reviewers: + thakis@chromium.org
thakis@ for OWNERS review
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.... base/compiler_specific.h:91: #define ALLOW_UNUSED_LOCAL(x) false ? (void)x : (void)0 What was the problem with making this use `(void)x` again?
marshallk@google.com changed reviewers: + marshallk@google.com
+danakj for alternative OWNER review
I'm not sure if exchanging owners after the first one you picked asked a question is productive. On Apr 25, 2017 1:07 PM, <marshallk@google.com> wrote: > +danakj for alternative OWNER review > > https://codereview.chromium.org/2838713002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Oh, sorry about that - got a stale page in Chrome and didn't see the reply. 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.... base/compiler_specific.h:91: #define ALLOW_UNUSED_LOCAL(x) false ? (void)x : (void)0 On 2017/04/25 15:48:52, Nico wrote: > What was the problem with making this use `(void)x` again? When I asked pkasting (author of the original #define), he said the ternary expression guarantees no side effects as a result of storing 'x'. I don't the specifics about which types are affected.
On Tue, Apr 25, 2017 at 1:17 PM, <marshallk@google.com> wrote: > Oh, sorry about that - got a stale page in Chrome and didn't see the reply. > > > 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 > On 2017/04/25 15:48:52, Nico wrote: > > What was the problem with making this use `(void)x` again? > > When I asked pkasting (author of the original #define), he said the > ternary expression guarantees no side effects as a result of storing > 'x'. I don't the specifics about which types are affected. > Right, I was hoping Peter could reply explaining why we need that. ALLOW_UNUSED_LOCAL is supposed to be used with local vars, not with expressions, so there shouldn't be any side effects. It looks like all uses of it do call it with just var names: https://cs.chromium.org/search/?q=allow_unused_local%5C(.*%5B%5Ea-z0-9_%5D.*%... So I wonder if we can just change ALLOW_UNUSED_LOCAL to (void)x. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
It's worth a shot. You'd want to make sure we don't generate worse code -- just compile each different way and check if the binaries are the same size. If so, I think it's OK. (I can't answer the question in much detail because the last time I touched this was years in the past.)
Will do a side by side comparison now. On Tue, Apr 25, 2017 at 11:55 AM, <pkasting@chromium.org> wrote: > It's worth a shot. You'd want to make sure we don't generate worse code -- > just > compile each different way and check if the binaries are the same size. If > so, > I think it's OK. > > (I can't answer the question in much detail because the last time I > touched this > was years in the past.) > > https://codereview.chromium.org/2838713002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The byte counts are identical for the current ternary expr #define and just using (void)x directly. It looks safe to remove the ternary expression, which is now done in the latest patch.
Description was changed from ========== Add ALLOW_UNUSED_LOCAL implementation for path-sensitive analyzers. 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 adds an analyzer-specific specialization of ALLOW_UNUSED_LOCAL which always evaluates (void)x. Evaluating "x" could potentially lead to side effects, but this is mitigated by the fact that analysis builds are generally run for analysis artifacts, not for their binaries. R=pkasting@chromium.org CC=wez@chromium.org BUG=687243 ========== to ========== 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 ==========
lgtm
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2838713002/#ps40001 (title: "Removed ternary expr & alternative impl for ALLOW_UNUSED_LOCAL")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Rad. Thanks for pushing on this, Nico, and doing the comparison, Kevin -- glad we could rip out unnecessary complexity :)
The CQ bit was unchecked by commit-bot@chromium.org
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_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493170693053830, "parent_rev": "79f22e60f857a83a592216c33a47035268c2cda7", "commit_rev": "d10bb7f7a2f052d8240079b43572c9c5124a20c3"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/d10bb7f7a2f052d8240079b43572... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d10bb7f7a2f052d8240079b43572... |