-
Notifications
You must be signed in to change notification settings - Fork 5
Refactoring to remove singleton ConstraintVariable sets #245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Did you ensure that this doesn't break vsftpd, both when running with -alltypes, -addcr, and without ? If so, I'll be more confident to take a look. |
All convert as expected. Without any flags it converts and compiles. With |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are just boiling down to a couple more name changes; check my comments to make sure you got them all.
This removes the need to create tempo Ry singleton sets or inline loops when the LHS of the call is a single ConstraintVariable.
Oops looks like there are conflicts to resolve (now). |
Name changes plus a few other tweaks are in and the conflict is resolved |
@@ -288,7 +288,7 @@ CVarSet | |||
// constraining GEQ these vars would be the cast always be WILD. | |||
if (!isNULLExpression(ECE, *Context)) { | |||
PersistentSourceLoc PL = PersistentSourceLoc::mkPSL(ECE, *Context); | |||
constrainConsVarGeq(Ret, Vars, Info.getConstraints(), &PL, | |||
constrainConsVarGeq(P, Vars, Info.getConstraints(), &PL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes me a little nervous -- why the name change? I'm not seeing (in this diff) a collateral change elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just above what's shown in the diff there is Ret = {P}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are good aside from the one comment that confuses me. If you solve that (maybe it's nothing) go ahead and merge.
Some of the
std::set<ConstraintVariable*>
sets we have are being used exclusively as singleton sets. This pull requests removes these sets, replacing them with a single ConstraintVariable pointer.The first two commits should not be controversial. These add
const
qualifiers onto method declarations to make it easier to identify where constraint variable sets can possibly be mutated. After adding theconst
s, I only have to look at the non-const
methods that access a set to determine if the set is used exclusively as a singleton set.The third commit adds
override
annotations to theConstraintVariable
class hierarchy. This helps ensure I haven't broken an virtual methods while adding consts to their signatures.The next three commits are the body of this pull requests. Each of the commits takes one
std::set<ConstraintVariable*>
field and replaces it with a single constraint variable pointer. These commits are mostly independent, so they can be looked at in isolation.