[PATCH] D82381: [analyzer] Introduce small improvements to the solver infra

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

Hi @vsavchenko , sorry for the late review.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:367-378
+RangeSet RangeSet::Delete(BasicValueFactory &BV, Factory &F,
+                          const llvm::APSInt &Point) const {
+  llvm::APSInt Upper = Point;
+  llvm::APSInt Lower = Point;
+
+  ++Upper;
+  --Lower;
----------------
Useful function. But I'd better rename it to `subtract` as we are working with sets (as a mathimatical collection). We should have a such one for the Ranges not only for Points.
We have `intersect`, `delete` aka `subtract`. And we also need to have functions `union` and `symmetricDifference` to cover full palette of common operations on sets.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:734
   //        expressions which we currently do not know how to negate.
-  const RangeSet *getRangeForMinusSymbol(ProgramStateRef State, SymbolRef Sym) {
+  Optional<RangeSet> getRangeForInvertedSub(SymbolRef Sym) {
     if (const SymSymExpr *SSE = dyn_cast<SymSymExpr>(Sym)) {
----------------
As for me, I'd call this like `getRangeForNegatedSymSymExpr`, since you do Negate operation inside.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:841-844
+  RangeSet getTrueRange(QualType T) {
+    RangeSet TypeRange = infer(T);
+    return assumeNonZero(TypeRange, T);
+  }
----------------
Don't you think this is too complicated for such a simple getter?
Maybe we can just construct the range using smth about `RangeSet(RangeFactory, ++Zero, --Zero);` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82381