vsavchenko marked 2 inline comments as done.
vsavchenko added inline comments.
================
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)) {
----------------
ASDenysPetrov wrote:
> 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.
My thought process here was that we are trying to get range for `A - B` and
there is also information on `B - A`, so we can get something for `A - B` based
on that. So, it doesn't really matter what it does under the hood with ranges,
it matters what its semantics are. Here I called `B - A` //an inverted
subtraction//.
I don't really know what would be the best name, but I thought that this one
makes more sense.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:841-844
+ RangeSet getTrueRange(QualType T) {
+ RangeSet TypeRange = infer(T);
+ return assumeNonZero(TypeRange, T);
+ }
----------------
ASDenysPetrov wrote:
> 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.
`infer(QualType T)` does just that ☺️ So it is a pretty low complexity.
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
