[PATCH] D26061: [analyzer] Refactor and simplify SimpleConstraintManager
Devin Coughlin via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 31 14:48:53 PDT 2016
dcoughlin added a comment.
Thanks for the patch!
Would it be possible to split this up into several patches? I think it is important to separate the interface layering changes from the formatting changes, renaming changes, and minor optimization changes. This will make the patches easier to review.
Also: is this motivated by a different implementation of range constraints? Is this something you plan on upstreaming?
In https://reviews.llvm.org/D26061#581778, @ddcc wrote:
> To summarize, here is a list of changes:
> - General
> - Fixed some issues with formatting (`clang-format`)
> - Fixed inconsistent capitalization following camel case style guidelines
> - `ConstraintManager.h`
> - Renamed `assumeWithinInclusiveRange*()` to `assumeInclusiveRange*()`, since the range is not necessarily contained within unless `Assumption` is true, to match `assume()`
> - `RangedConstraintManager.h` (inherits `SimpleConstraintManager`)
> - Moved `assumeSym*` and `canReasonAbout()` from `SimpleConstraintManager` to here
> - Renamed `assumeSymbolWithinInclusiveRange`/`assumeSymbolOutOfInclusiveRange` to `assumeSymWithinInclusiveRange`/`assumeSymOutsideInclusiveRange` to match the above, and moved to here
> - This is now inherited by `RangeConstraintManager`, and essentially provides the current interface in `SimpleConstraintManager`
What do you view as the distinction between RangeConstraintMananger and RangedConstraintManager? In other words, if I'm a developer adding new functionality how would you suggest I decide which to add it to? It would be good to document this in the code! Also: the closeness in the names of these classes could be potentially confusing. Are there other, more distinctive, names that still communicate the difference you intend?
> - `SimpleConstraintManager.h` (inherits `ConstraintManager`)
> - Implemented a new interface that internally converts the `DefinedSVal` in `assume(...)` from `ConstraintManager.h` to `NonLoc` to `SymbolRef`. Subclasses only need to override `ProgramStateRef assumeSym(ProgramStateRef State, SymbolRef Sym, bool Assumption)`
> - For inclusive ranges, implemented a new interface that internally converts from `NonLoc` to `SymbolRef`. Subclasses only need to override `ProgramStateRef assumeSymInclusiveRange(ProgramStateRef State, SymbolRef Sym, const llvm::APSInt &From, const llvm::APSInt &To, bool InRange)`
> - Implemented a new interface that internally handles expressions that fail `canReasonAbout()` by adding them directly to the constraint manager state. Subclasses only need to expose `ProgramStateRef assumeSymRel(ProgramStateRef State, SymbolRef Sym, BinaryOperator::Opcode op, const llvm::APSInt &Int)`
> - `RangeConstraintManager.cpp`
> - Minor optimization to avoid updating the state if nothing is pruned in `removeDeadBindings()`
More information about the cfe-commits