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

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware 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
----------------
ASDenysPetrov wrote:
> Can we use `Optional<BinaryOperatorKind>` instead, to reduce similar enums? Or you want to separate the meaning in a such way?
Good question. @NoQ?


================
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());
----------------
ASDenysPetrov wrote:
> As you use here `getMaxValue` twice which is not O(1), it'd better to use a buffer variable.
`getMaxValue()` for the current and `other.getMaxValue()` are different.


================
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();
+}
----------------
ASDenysPetrov wrote:
> Can we improve `llvm::ImmutableSet` this to make `getMaxValue` complexity O(1)?
`llvm::ImmutableSet` seems to me a //heap//-like structure, a tree optimized for minimum-search: the mininum is in the root of the tree. Maximum is linear.


================
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));
----------------
ASDenysPetrov wrote:
> Could you add some tests for `unsigned, unsigned` and `signed, unsigned`?
I will do it.


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

https://reviews.llvm.org/D77792