[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked 2 inline comments as done.
rtrieu added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8161
+  "bitwise or with non-zero value always evaluates to true">,
+  InGroup<TautologicalBitwiseCompare>, DefaultIgnore;
 def warn_tautological_overlap_comparison : Warning<
----------------
jfb wrote:
> rtrieu wrote:
> > jfb wrote:
> > > Why default ignore?
> > 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.
> Can you say so in the commit message?
Updated patch description.


================
Comment at: lib/Analysis/CFG.cpp:1118
+    Expr::EvalResult Result;
+    if (!Constant->EvaluateAsInt(Result, *Context))
+      return {};
----------------
NoQ wrote:
> 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?
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66046/new/

https://reviews.llvm.org/D66046