[PATCH] D65239: [analyzer] RangeConstraintManager: Apply constraint ranges of bitwise operations

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:496
+  // as a bitwise operation result could be null.
+  if (RS.getConcreteValue() && RS.getConcreteValue()->getExtValue() == 0)
+    return State;
----------------
NoQ wrote:
> Instead of "we know that the value is null", we should write "we //don't// know that the value is //non-//null". I.e. if we're not sure, we must still do an early return.
Oh, fail. Thanks!


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:507
+  ConstraintRangeTy Constraints = State->get<ConstraintRange>();
+  for (const SymbolRef CurrentSym : SIE->symbols()) {
+    if (CurrentSym == SIE)
----------------
NoQ wrote:
> This loop doesn't make sense. When your expression looks like `(((a + b) + c) + d) & (e + f)`, you don't want to iterate separately over `a`, `b`, `c`, `d`, `e`, `f`; but that's what this loop would do. You should only look at the LHS and the RHS. If you want to descend further, do so recursively, so that to keep track of the overall structure of the symbolic expression as you're traversing it, rather than traversing sub-expressions in an essentially random order.
It is rather sequential in the bitwise world, but I think if you would look only at the LHS `SymExpr`, it should be enough. Also I would like to avoid extra overhead, that `symbols()` business was large enough. Thanks!


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

https://reviews.llvm.org/D65239