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

Philip Craig philipjcraig at gmail.com
Tue Sep 25 20:14:10 PDT 2012


On Wed, Sep 26, 2012 at 2:18 AM, Jordan Rose <jordan_rose at apple.com> wrote:
>
> 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.

Is there a reason why the syntactic form is split up like that? Is it
possible to have only two child expressions for PseudoObjectExpr,
syntactic and semantic, where the syntactic form doesn't have any
OpaqueValueExprs, and the OpaqueValueExprs in the semantic form point
within the syntactic form. If this was possible, then the ParentMap
wouldn't need any special handling for OpaqueValueExpr at all. (I'm
not familiar with codegen or the use of PseudoObjectExpr, so this
sugggestion may be way off.)

> 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
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev




More information about the cfe-dev mailing list