martong marked 2 inline comments as done.
martong added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2314-2315
+ if (SymbolRef SimplifiedSym = simplify(St, Sym))
+ Sym = SimplifiedSym;
+
----------------
vsavchenko wrote:
> martong wrote:
> > vsavchenko wrote:
> > > I don't like the idea of duplicating it into every `assume` method. This
> > > way we drastically increase our chances to forget it (like you did with
> > > `assumeSymGE` and `assumeSymLE`).
> > > I think the better place for it is in
> > > `RangedConstraintManager::assumeSymRel` and neighboring methods, though
> > > still not perfect.
> > > I don't really get why we get not simplified symbol to begin with.
> > >
> > `assumeSymRel` is not enough, because e.g. `assumeSymGE` is called also
> > e.g. from `assumeSymUnsupported`. Perhaps we could change the signature of
> > `assumeSymEQ/NE/GT/GE/LT/LE` to take an auxiliary `Simplifier` wrapper
> > object instead of `SymbolRef`?
> >
> > ```
> > ProgramStateRef assumeSymNE(ProgramStateRef State, Simplifier S,
> > const llvm::APSInt &V,
> > const llvm::APSInt &Adjustment);
> >
> > ```
> > And for the Simplifier something like:
> > ```
> > struct Simplifier {
> > SymbolRef SimplifiedSym = nullptr;
> > Simplifier(SymbolRef Sym) : SimplifiedSym(simplify(Sym)) {}
> >
> > };
> > ```
> >
> > assumeSymRel is not enough, because e.g. assumeSymGE is called also e.g.
> > from assumeSymUnsupported.
> Yep, that's why I suggested `assumeSymRel` and its neighbors. I actually
> think that three top-level public methods from `RangedConstraintManager` will
> do: `assumeSym`, `assumeSymInclusiveRange`, and `assumeSymUnsupported`.
>
>
> We can't really change the signatures of those methods because we'll be
> introducing this functionality into solvers that didn't sign up for this (and
> don't need it).
>
> Also we can least put this `if` statement inside of `simplify`, so we can use
> it like this: `Sym = simplify(St, Sym);`.
Okay, I've updated like so.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104844/new/
https://reviews.llvm.org/D104844
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
