[cfe-dev] r161350 breaks the intention of ParentMap::addStmt

[Jordan Rose]

On Sep 25, 2012, at 3:41 , Olaf Krzikalla <Olaf.Krzikalla at tu-dresden.de> wrote:

>> Perhaps rather than just looking through OpaqueValueExprs, we should
>> just explicitly handle their use cases in ParentMap?
>> - BinaryConditionalOperator
>> - PseudoObjectExpr, currently used for ObjCPropertyRefExpr and
>> ObjCSubscriptRefExpr
>> - C++ 'catch' variables, as a placeholder
>> (Sema::BuildExceptionDeclaration) <-- I'd be okay with not worrying
>> about this one, since it just contains a DeclRefExpr back to the
>> variable to make it look initialized.
>> 
>> What do you think?
> Even if I don't like the current design of OpaqueValueExprs (assuming that I've understood correctly, that a source expr can have multiple OpaqueValueExpr parents) I'm completely OK with the handling of it just because it doesn't bother me.
> For PseudoObjectExpr we could just look whether the parent is a PseudoObjectExpr (see the patch) and do a special handling in that case. However I don't understand while it is harmful if the semantic children have their PseudoObjectExpr as parent.

Sorry, it isn't PseudoObjectExpr that's the problem; it's OpaqueValueExpr. (I just realized that now, meaning my comment is wrong.) The source expr of an OpaqueValueExpr isn't directly accessible as a child to avoid multiple traversal, but the rest of that commit taught ParentMap how to look through an OpaqueValueExpr. That probably isn't the right thing to do.

To illustrate the problem, here are the pieces inside 'self.property = value' (roughly):

0: $1.property = $2
1: 'self'
2: 'value'
3: [$1 setProperty:$2]

$1 and $2 represent OpaqueValueExprs that refer back to the real expressions in child slots 1 and 2. Child slot 0 is the "syntactic" form and is not used by CodeGen or the analysis engine, but it /is/ used for diagnostics. For the analyzer to print the best diagnostics, we want the parent expr of 'value' to be the assignment expression (0), and the parent expr of 'self' to be the ObjCPropertyRefExpr /inside/ the assignment ($1.property). Having the parent be the entire PseudoObjectExpr is possible but less than ideal.

I'm convincing myself more and more that this just means we should handle the /users/ of OpaqueValueExprs specially rather than trying to write a magic bullet solution that handles all OpaqueValueExprs. (For the BinaryConditionalOperator, should the parent of the LHS be the implicit cast or the entire operator?)

Sorry to keep drawing this out,
Jordan