SimpleConstraintManager is difficult to use, and makes assumptions about capabilities of the constraint manager. This patch refactors out those portions into a new RangedConstraintManager, and also fixes some issues with camel case, formatting, and confusing naming.
Details
Diff Detail
- Build Status
Buildable 3118 Build 3118: arc lint + arc unit
Event Timeline
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
- 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()
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?
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()
Yes, I've been writing a Z3 solver interface, which motivated this patch. However, this patch has snowballed into something that it's a little too convoluted, so I'll split it up.
I'm not sure whether the RangedConstraintManager interface is useful or not; I preserved it because it's currently in the code, but since RangeConstraintManager is the only user, it is possible to merge the two together and eliminate the interface. In the past, BasicConstraintManager was the other class that used this interface, but that was deleted quite a while back, and I'm not sure if there are plans for anything else?
Rebase, move assumeSymRel() to RangedConstraintManager, make assumeSymUnsupported pure virtual in SimpleConstraintManager.