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

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov marked 6 inline comments as done.
ASDenysPetrov added a comment.

@martong Thanks for review.



================
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));
+
----------------
martong wrote:
> 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`.
Yes. Definetely. It remained from my previous version of calculation.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:281
+    RangeSet Result = F.invert(What);
+    EXPECT_EQ(Result, Expected) << "while inverting " << toString(What);
+  }
----------------
martong wrote:
> 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.
Great idea. I will do it, except `[]`.


================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:1130
+  // Check inverting single range.
+  this->checkInvert({{MIN, MAX}}, {});
+  this->checkInvert({{MIN, MID}}, {{MID + 1, MAX}});
----------------
martong wrote:
> 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. 
I'm afraid the function would be overheaded and less usable. Why do we have an assertion here? I thought about this. This is kinda a tradeoff.

What range would be a correct range from inversion of `[]`? `[-128, 127]`? `[0, 65535]`?
In order to make `[MIN, MAX]` from empty range `[]` we should know the exact `APSIntType`. We extract the type from the given range `What.getAPSIntType();`.

But though, let's consider we have `QualType` as a second parameter. We have two options about what type to use.
1. Always use the second parameter. The function would not only invert but do what `castTo` actually do. Such behavior would violates a single responsibility principle and duplicate the functionality.
2. Always use the type from the given range and for the case of empty range from the second parameter. The function contract would be more complex and obscure.

So, I decided not to introduce a new parameter and shift responsibility to the user. Empty range is an edge case when we usually produce infeasible state or use `infer(QualType)`. This may pretty fit with what user is going to do first before using `invert`, so it shouldn't affect the convinience of function usage too much.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130372