[PATCH] D77792: [analyzer] Extend constraint manager to be able to compare simple SymSymExprs

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

I like it, nice work!



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:774
 
+Optional<ProgramStateRef> RangeConstraintManager::tryAssumeSymSymOp(
+    ProgramStateRef State, BinaryOperator::Opcode Op, SymbolRef LHSSym,
----------------
Is it possible to ever return with `None`? Other `assume` functions usually just return with `nullptr` on infeasible state as you do. What would be the meaning if `None` is returned, is that another kind of infeasibility?


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:797
+
+  switch (Op) {
+  case BO_LT:
----------------
Perhaps this hunk could be greatly simplified if `CompareResult` was actually `BinaryOperator::Opcode`.

Maybe (?):
```
if (Res == Op)
  return State;
return InfeasibleState;
```


================
Comment at: clang/test/Analysis/constraint-manager-sym-sym.c:70
+
+// [2,5] and [5,10]
+void test_range6(int l, int r) {
----------------
How is it different than `// [0,5] and [5,10]`?


================
Comment at: clang/test/Analysis/constraint-manager-sym-sym.c:172
+}
+
----------------
I think we could benefit from tests for cases like this: 
```
{[0,1],[5,6]} < {[9,9],[11,42],[44,44]}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77792