ASDenysPetrov added a comment.
@vsavchenko
Thank you.
Despite of all of my nits, LGTM!
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:367-378
+RangeSet RangeSet::Delete(BasicValueFactory &BV, Factory &F,
+ const llvm::APSInt &Point) const {
+ llvm::APSInt Upper = Point;
+ llvm::APSInt Lower = Point;
+
+ ++Upper;
+ --Lower;
----------------
vsavchenko wrote:
> ASDenysPetrov wrote:
> > Useful function. But I'd better rename it to `subtract` as we are working
> > with sets (as a mathimatical collection). We should have a such one for the
> > Ranges not only for Points.
> > We have `intersect`, `delete` aka `subtract`. And we also need to have
> > functions `union` and `symmetricDifference` to cover full palette of common
> > operations on sets.
> I agree that we should have a full set of functions. I don't agree however,
> that this function is a `subtract`. Subtract is an operation on two sets and
> here we have a set and a point. One might argue that a point is just a very
> simple set, that's true, but real `subtract` would be more complex in its
> implementation.
>
> Naming it `delete`, on the other hand, I was coming from a notion of deleting
> points or neighbourhoods
> (https://en.wikipedia.org/wiki/Neighbourhood_(mathematics)#Deleted_neighbourhood).
>One might argue that a point is just a very simple set
That's actually what I mean :) and for this particular case you may leave the
implementation as is. And for me it still does what `subtract` does. But I'm
OK, I don't insist.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:734
// expressions which we currently do not know how to negate.
- const RangeSet *getRangeForMinusSymbol(ProgramStateRef State, SymbolRef Sym)
{
+ Optional<RangeSet> getRangeForInvertedSub(SymbolRef Sym) {
if (const SymSymExpr *SSE = dyn_cast<SymSymExpr>(Sym)) {
----------------
vsavchenko wrote:
> ASDenysPetrov wrote:
> > As for me, I'd call this like `getRangeForNegatedSymSymExpr`, since you do
> > Negate operation inside.
> I'm not super happy about my name either, but I feel like it describes it
> better than the previous name and your version. That function doesn't work
> for any `SymSymExpr` and it doesn't simply negate whatever we gave it. It
> works specifically for symbolic subtractions and this is the information I
> want to be reflected in the name.
Oh, I just assumed //...Sub// at the end as a //subexpression// but you mean
//subtraction//. What I'm trying to say is that we can rename it like
`getRangeFor...`//the expression which this function can handle//. E.g.
`getRangeForNegatedSubtractionSymSymExpr`. My point is in a speaking name.
I think //invertion// is not something appropriate in terms of applying minus
operator. I think invertion of zero should be something opposite but not a
zero. Because when you would like to implement the function which turns [A, B]
into [MIN, A)U(B, MAX], what would be the name of it? I think this is an
//invertion//.
But this is not a big deal, it's just my thoughts.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:841-844
+ RangeSet getTrueRange(QualType T) {
+ RangeSet TypeRange = infer(T);
+ return assumeNonZero(TypeRange, T);
+ }
----------------
vsavchenko wrote:
> ASDenysPetrov wrote:
> > Don't you think this is too complicated for such a simple getter?
> > Maybe we can just construct the range using smth about
> > `RangeSet(RangeFactory, ++Zero, --Zero);` ?
> It is more complex than a false range but there is a reason for it.
>
> First of all, `RangeSet` can't have ranges where the end is greater than its
> start. Only `Intersect` can handle such ranges correctly. Another thing is
> that ranges like that mean `[MIN, --Zero], [++Zero, MAX]` and without a type
> we can't really say what `MIN` and `MAX` are, so such constructor for
> `RangeSet` simply cannot exist.
>
> Another point is that we do need to have `[MIN, -1], [+1, MAX]` as opposed to
> `[-1, -1], [+1, +1]` because of C language (it doesn't have boolean type),
> and because of the cases like `a - b` where we know that `a != b`.
>
> I hope that answers the question.
I just want this function has low complexity and be more lightweight as
`getFalseRange`. And if we have any chance to simplify it (and you have all the
data to get MIN and MAX), it'd be cool.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82381/new/
https://reviews.llvm.org/D82381
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
