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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D130372#3683275 <https://reviews.llvm.org/D130372#3683275>, @ASDenysPetrov wrote:

> Now I'm working on the next patch and you'll see a motivation soon.

Thanks for the update! Looks good, but I am still waiting with my formal approve to understand the motivation.



================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:1130
+  // Check inverting single range.
+  this->checkInvert({{MIN, MAX}}, {});
+  this->checkInvert({{MIN, MID}}, {{MID + 1, MAX}});
----------------
ASDenysPetrov wrote:
> 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.
Okay, I see your point.

So, considering `RangeSet::Factory::invert(RangeSet What)` we have only one parameter and that is the RangeSet to be inverted. And that set might be empty and in that case we are left without any type information, thus there is no way to return [MIN, MAX]. Please correct me if I am wrong.


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

https://reviews.llvm.org/D130372