Fix macro expansion bug #266
Conversation
These were previously stored the Variables map that holds declaration constraint variables.
| @@ -498,8 +498,7 @@ class VariableAdderVisitor : public RecursiveASTVisitor<VariableAdderVisitor> { | |||
|
|
|||
| bool VisitVarDecl(VarDecl *D) { | |||
| FullSourceLoc FL = Context->getFullLoc(D->getBeginLoc()); | |||
| if (FL.isValid() && | |||
| (!isa<ParmVarDecl>(D) || D->getParentFunctionOrMethod() != nullptr)) | |||
| if (FL.isValid() && !isa<ParmVarDecl>(D)) | |||
There was a problem hiding this comment.
Was the getParentFunctionOrMethod redundant?
There was a problem hiding this comment.
I'm not sure what the original motivation for adding that clause to the conditional. getParentFunctionOrMethod returns true when the ParmVarDecl is associated with a function, but if it's associated with a function, it's added to the variable map at the same time as the function declaration. I thought maybe it was supposed to be == nullptr (i.e., it does not have an associated function), but this breaks lots of things.
| @@ -375,7 +375,8 @@ PointerVariableConstraint::PointerVariableConstraint(const QualType &QT, | |||
| // inside a macro. Not sure how to handle this, so fall back to tyToStr. | |||
| if (BaseType.empty()) | |||
| FoundMatchingType = false; | |||
| } | |||
| } else | |||
| FoundMatchingType = false; | |||
There was a problem hiding this comment.
Seems odd to me that FoundMatchingType is the default, rather than the exception. There are multiple places you set it as false; would it make sense to default to false and set it to true when sure?
| @@ -217,7 +217,8 @@ bool CheckedRegionFinder::VisitMemberExpr(MemberExpr *E){ | |||
| if (VD) { | |||
| // Check if the variable is WILD. | |||
| CVarOption Cv = Info.getVariable(VD, Context); | |||
| if (Cv && (*Cv)->hasWild(Info.getConstraints().getVariables())) | |||
| if (Cv.hasValue() | |||
| && Cv.getValue().hasWild(Info.getConstraints().getVariables())) | |||
Great! I've got one or two tweaks to commit and then I'll merge tomorrow morning. |
This pull request is a fix for the macro expansion bug in issues #255 and #18 . This bug was caused because by collisions between persistent source locations. Using our current source location representation, it is possible to for two distinct objects to reside at the same source location. The fix I've implemented does not directly address this. Instead it improves the how we handle situations when such a collision is found.
When a source location collision is found, the affected constraint variables are immediately constrained to wild, and a single wild constraint variable is stored in the variables map. Forcing these constraints to be wild is fine because source location collisions occur inside macros, which are always wild anyways. This avoids inconsistent handling of non-singleton constraint variable sets in the
Variablesmap.As part of this fix, the
Variablesmap inProgramInfohas been updated to map source locations to a singleConstraintVariable *instead of astd::set<ConstraintVariable *>. This reflects how the map is actually used. Somewhat recently we began storing constraints for expressions in this set instead of using it only for declaration constraints. This change is problematic because expressions (unlike declaration) have non-singleton sets of constraint variables. I have created a new map for storing these constraint variables sets. I think this is a generally good organizational change because the declaration and expression constraint variables are used for very different purposes in the code (rewriting declaration vs. caching pre-computed constraint sets).