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