Page MenuHomePhabricator

Add new tautological compare warning for bitwise-or with a non-zero constant
AcceptedPublic

Authored by rtrieu on Fri, Aug 9, 6:54 PM.

Details

Reviewers
cfe-commits
NoQ
Summary

Taking a value and the bitwise-or it with a non-zero constant will always result in a non-zero value. In a boolean context, this is always true.

if (x | 0x4) {} // always true, intended '&'

This patch creates a new warning group -Wtautological-bitwise-compare for this warning. It also moves in the existing tautological bitwise comparisons into this group. A few other changes were needed to the CFGBuilder so that all bool contexts would be checked. The warnings in -Wtautological-bitwise-compare will be off by default due to using the CFG.

https://bugs.llvm.org/show_bug.cgi?id=42666

Diff Detail

Event Timeline

rtrieu created this revision.Fri, Aug 9, 6:54 PM
NoQ added a reviewer: NoQ.Fri, Aug 9, 8:18 PM
jfb added a subscriber: jfb.Sat, Aug 10, 8:52 AM
jfb added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
8161

Why default ignore?

rtrieu marked an inline comment as done.Mon, Aug 12, 2:50 PM
rtrieu added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
8161

This warning, like the tautological overlap warning, uses the CFG. CFG-based analysis are typically excluded from being default warnings due to the extra work of construction the CFG.

jfb added inline comments.Mon, Aug 12, 2:53 PM
include/clang/Basic/DiagnosticSemaKinds.td
8161

Can you say so in the commit message?

rtrieu edited the summary of this revision. (Show Details)Mon, Aug 12, 3:29 PM
NoQ added inline comments.Mon, Aug 12, 3:31 PM
lib/Analysis/CFG.cpp
1132

It's kinda strange to me that we first confirm that it's a constant by doing tryTransformToIntOrEnumConstant, but then fire up the heavy machinery of EvaluateAsInt anyway. Did you consider using only EvaluateAsInt() to begin with? I guess you're trying to make sure that "the user's intent is clear" as other similar warnings do, right? Could you comment on that?

rtrieu updated this revision to Diff 214733.Mon, Aug 12, 4:18 PM

Create new function isIntOrEnumConstantZero to use instead of EvaluateAsInt

rtrieu marked 2 inline comments as done.Mon, Aug 12, 4:21 PM
rtrieu added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
8161

Updated patch description.

lib/Analysis/CFG.cpp
1132

A new function has been created to check the zero-ness of the Expr. Since we know it the Expr comes from tryTransformToIntOrEnumConstant, this function doesn't have to handle the full Expr sub-classes.

NoQ accepted this revision.Fri, Aug 16, 4:32 PM

Looks great, thank you!

lib/Analysis/CFG.cpp
1132

Whoa!

This revision is now accepted and ready to land.Fri, Aug 16, 4:32 PM

You should also probably add a note in the release notes (maybe for the others changes too)
thanks for the work btw!