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

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:103-110
+  enum class CompareResult {
+    unknown,
+    identical,
+    less,
+    less_equal,
+    greater,
+    greater_equal
----------------
Can we use `Optional<BinaryOperatorKind>` instead, to reduce similar enums? Or you want to separate the meaning in a such way?


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:30-33
+  const int LMaxRMin =
+      llvm::APSInt::compareValues(getMaxValue(), other.getMinValue());
+  const int LMinRMax =
+      llvm::APSInt::compareValues(getMinValue(), other.getMaxValue());
----------------
As you use here `getMaxValue` twice which is not O(1), it'd better to use a buffer variable.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:106-113
+const llvm::APSInt &RangeSet::getMaxValue() const {
+  assert(!isEmpty());
+  auto last = ranges.begin();
+  for (auto it = std::next(ranges.begin()); it != ranges.end(); ++it)
+    last = it;
+  return last->To();
+}
----------------
Can we improve `llvm::ImmutableSet` this to make `getMaxValue` complexity O(1)?


================
Comment at: clang/test/Analysis/constraint-manager-sym-sym.c:181
+// {[0,1],[20,20]} and {[9,9],[11,42],[44,44]}
+void test_range18(int l, int r) {
+  assert((9 <= r && r <= 9) || (11 <= r && r <= 42) || (44 <= r && r <= 44));
----------------
Could you add some tests for `unsigned, unsigned` and `signed, unsigned`?


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

https://reviews.llvm.org/D77792