r182187 - [analyzer] "Fix" ParentMap to handle non-syntactic OpaqueValueExprs.

[Jordan Rose]

On May 17, 2013, at 19:45 , Matt Beaumont-Gay <matthewbg at google.com> wrote:

> On Fri, May 17, 2013 at 7:27 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>> Author: jrose
>> Date: Fri May 17 21:27:09 2013
>> New Revision: 182187
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=182187&view=rev
>> Log:
>> [analyzer] "Fix" ParentMap to handle non-syntactic OpaqueValueExprs.
>> 
>> Constructs like PseudoObjectExpr, where an expression can appear more than
>> once in the AST, use OpaqueValueExprs to guard against inadvertent
>> re-processing of the shared expression during AST traversal. The most
>> common form of this is to share expressions between the syntactic
>> "as-written" form of, say, an Objective-C property access 'obj.prop', and
>> the underlying "semantic" form '[obj prop]'.
>> 
>> However, some constructs can produce OpaqueValueExprs that don't appear in
>> the syntactic form at all; in these cases the ParentMap wasn't ever traversing
>> the children of these expressions. This patch fixes that by checking to see
>> if an OpaqueValueExpr's child has ever been traversed before. There's also a
>> bit of reset logic when visiting a PseudoObjectExpr to handle the case of
>> updating the ParentMap, which some external clients depend on.
>> 
>> This still isn't exactly the right fix because we probably want the parent
>> of the OpaqueValueExpr itself to be its location in the syntactic form if
>> it's syntactic and the PseudoObjectExpr or BinaryConditionalOperator itself
>> if it's semantic. Whe I originally wrote the code to do this, I didn't realize
>> that OpaqueValueExprs themselves are shared in the AST, not just their source
>> expressions. This patch doesn't change the existing behavior so as not to
>> break anything inadvertently relying on it; we'll come back to this later.
>> 
>> Modified:
>>    cfe/trunk/lib/AST/ParentMap.cpp
>> 
>> Modified: cfe/trunk/lib/AST/ParentMap.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ParentMap.cpp?rev=182187&r1=182186&r2=182187&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/AST/ParentMap.cpp (original)
>> +++ cfe/trunk/lib/AST/ParentMap.cpp Fri May 17 21:27:09 2013
>> @@ -33,6 +33,11 @@ static void BuildParentMap(MapTy& M, Stm
>>     assert(OVMode == OV_Transparent && "Should not appear alongside OVEs");
>>     PseudoObjectExpr *POE = cast<PseudoObjectExpr>(S);
>> 
>> +    // If we are rebuilding the map, clear out any existing state.
>> +    if (M[POE->getSyntacticForm()])
>> +      for (Stmt::child_range I = S->children(); I; ++I)
>> +        M[*I] = 0;
>> +
>>     M[POE->getSyntacticForm()] = S;
>>     BuildParentMap(M, POE->getSyntacticForm(), OV_Transparent);
>> 
>> @@ -62,13 +67,19 @@ static void BuildParentMap(MapTy& M, Stm
>> 
>>     break;
>>   }
>> -  case Stmt::OpaqueValueExprClass:
>> -    if (OVMode == OV_Transparent) {
>> -      OpaqueValueExpr *OVE = cast<OpaqueValueExpr>(S);
>> +  case Stmt::OpaqueValueExprClass: {
>> +    // FIXME: This isn't correct; it assumes that multiple OpaqueValueExprs
>> +    // share a single source expression, but in the AST a single
>> +    // OpaqueValueExpr is shared among multiple parent expressions.
>> +    // The right thing to do is to give the OpaqueValueExpr its syntactic
>> +    // parent, then not reassign that when traversing the semantic expressions.
>> +    OpaqueValueExpr *OVE = cast<OpaqueValueExpr>(S);
>> +    if (OVMode == OV_Transparent || !M[OVE->getSourceExpr()]) {
> 
> Some unicode garbage snuck in here...

Sorry about that! I just looked and Clang actually gave me a warning ("treating Unicode character as whitespace"). Unicode character support continues to haunt me even now.