[PATCH] D85287: Extend -Wtautological-bitwise-compare "bitwise or with non-zero value" warnings

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a subscriber: AndersRonnholm.
rtrieu added a comment.

I looked back on the commits and while I did commit, I did it on the behalf of Anders Rönnholm, who did not have commit access at the time.  I haven't seen activity from Anders in a while, but added to subscribers just in case.

Way back then, the warning only did operands of a DeclRefExpr and an IntegerLiteral.  Over time, that has been extended, case by case, to include whatever new cases people can think up.  I don't mind extending the warnings, but we need to be mindful of how the warnings appear.  If the sub-expression becomes too large, it will be difficult for the user to understand where the problem is and which constants the compiler is talking about.  We may already be at that point.  The example could have a more complex initializer for the constant variables, and the warning would be harder to follow.  Maybe we also look at the variable initializers and only allow for simple ones.  I need to give this some more thought.

> To avoid potential further false positives, restrict this change only to the
> "bitwise or with non-zero value" warnings while keeping all other
> -Wtautological-bitwise-compare warnings as-is, at least for now.

Are you planning to allow this change to other warnings that use the same helper functions?
Also, have you tried running warning over a codebase?



================
Comment at: clang/lib/Analysis/CFG.cpp:96-97
 
-/// Helper for tryNormalizeBinaryOperator. Attempts to extract an IntegerLiteral
-/// constant expression or EnumConstantDecl from the given Expr. If it fails,
+/// Helper for tryNormalizeBinaryOperator. Attempts to extract a suitable
+/// integer or enum constant from the given Expr. If it fails,
 /// returns nullptr.
----------------
The original comment specifies the allowed Expr's by the specific AST nodes they represent.  Please use that.  I think "IntegerLiteral constant expression, EnumConstantDecl, or constant value VarDecl" would work.


================
Comment at: clang/lib/Analysis/CFG.cpp:110
+      if (VD->isUsableInConstantExpressions(Ctx))
+        return DR;
+  }
----------------
IntergerLiteral and EnumConstantDecl are known to have integer types.  However, it is possible for a VarDecl to have other types.  There should be a check for integer types here.


================
Comment at: clang/lib/Analysis/CFG.cpp:175
 
-  assert(isa<EnumDecl>(DC1) && isa<EnumDecl>(DC2));
-  return DC1 == DC2;
+  return false;
 }
----------------
Need a comment here about how tryTransformToIntOrEnumConstant also allows a DeclRefExpr to have a constant VarDecl, but this case is currently excluded for this warning.


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

https://reviews.llvm.org/D85287