[PATCH] D86465: [analyzer][solver] Redesign constraint ranges data structure

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

I didn't check correctness of each Factory function but if it passes all tests and improves performance, I think it's enough.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:77-78
+  //
+  //   * Range sets are usually very simple, 1 or 2 ranges.
+  //     That's why llvm::ImmutableSet is not perfect.
+  //
----------------
That's very usefull description for reviewers, but I think is redundant for newcomers, when you're reading this you can't understand why it compares to `ImmutableSet` at all. I think this preamble only relates to the Summary of this patch as the core reason of this change.
You can just mention the fact that formerly it was an `ImmutableSet`. See //http// for details.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:133
+    ///             where N = size(Original)
+    RangeSet add(RangeSet Original, Range Element);
+
----------------
We can add one more instance of `add` (and maybe others): `RangeSet add(RangeSet original, llvm::APSInt &point);`.
Since we have `RangeSet(Factory &F, const llvm::APSInt &Point)` ctor, why wouldn't we have similar functionality in the `Factory`?

It could simplify function `RangeSetTest::from` in tests and be useful across the code on the whole.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:178
+    ///             where N = size(From)
+    RangeSet deletePoint(const llvm::APSInt &Point, RangeSet From);
+    /// Negate the given range set.
----------------
IMHO you should change params with each other:
`deletePoint(const llvm::APSInt &Point, RangeSet From)` -> `deletePoint(RangeSet Original, const llvm::APSInt &Point)`
This will look more consistent with other interfaces like in `add` or `intersect` functions.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:105
+  void checkNegate(RawRangeSet RawOriginal, RawRangeSet RawExpected) {
+    wrap(&Self::checkNegateImpl, RawOriginal, RawExpected);
+  }
----------------
Explain, please, what's the matter to use `wrap` everywhere, not just call `checkNegateImpl` explicitly?
Won't the call works and looks the same? Like:
```
this->checkNegateImpl({{MIN, A}}, {{MIN, MIN}, {D, MAX}});
```


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:185
+
+using IntTypes = ::testing::Types<int8_t, uint8_t, int16_t, uint16_t, int32_t,
+                                  uint32_t, int64_t, uint64_t>;
----------------
Thank you for making it in a proper way, instead of a list with different type instantiations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86465