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