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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
