[PATCH] D130372: [analyzer] Add a new factory function RangeSet::Factory::invert

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> The motivation is to extend the set of operations that can be performed on range sets. Specifically, this function is needed for the next patch in the stack.

Would be great to see the motivation clearly and early. What will be the next patch about?



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:711
 
+RangeSet RangeSet::Factory::invert(RangeSet What) {
+  assert(!What.isEmpty());
----------------
Could you please document this function and its complexity? (O(N) where N is the number of elements of the RangeSet.)


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:728-729
+  ContainerType Result;
+  Result.reserve(What.size() + 1 - bool(What.getMinValue() != MIN) -
+                 bool(What.getMaxValue() != MAX));
+
----------------
There might be a flaw here. Should this be `==` instead of `!=` ?
Consider e.g. when `What` has two elements `[[MIN, -1], [1, MAX]]` then the inverse should be `[0,0]` which has size `1`.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:734-735
+
+  if (It->From() == MIN)
+    goto loop;
+
----------------
No `goto` please. There are better alternatives, e.g. you could have two different `while` loops for each cases.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:281
+    RangeSet Result = F.invert(What);
+    EXPECT_EQ(Result, Expected) << "while inverting " << toString(What);
+  }
----------------
I think, it would make sense to test the composition of two invert operations to identity. 
```
EXPECT_EQ(What, F.invert(F.invert(What));
```
This, of course requires that empty set is a possible input.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:1101
 
+TYPED_TEST(RangeSetTest, RangeSetInvertTest) {
+  using TV = TestValues<TypeParam>;
----------------
Good tests!


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:1130
+  // Check inverting single range.
+  this->checkInvert({{MIN, MAX}}, {});
+  this->checkInvert({{MIN, MID}}, {{MID + 1, MAX}});
----------------
I'd expect that inversion on finite sets is an invert-able function thus
```
this->checkInvert({}, {MIN, MAX});
```
would make sense instead of assertion.

Besides, it would make sense to test the composition of two invert operations to identity. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130372