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

[Olaf Krzikalla]

Am 24.09.2012 19:49, schrieb Jordan Rose:
>>> A PseudoObjectExpr is used when we want one expression to essentially
>>> rewrite to another one.
>> Hmm, I did a textual search for PseudoObjectExpr, but haven't found it
>> anywhere but in the one comment in ParentMap.cpp. Looks as if it is
>> gone again. Maybe in that case the first part (causing the troubles)
>> of BuildParentMap can be reverted anyway(?)
>
> No, no.
Haha, I copy-pasted "PseudoObjectExprs" from the comment and searched 
for that. No wonder that I didn't found anything with the trailing 's'.

> 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.

Best regards
Olaf

-------------- next part --------------
Index: lib/AST/ParentMap.cpp
===================================================================
--- lib/AST/ParentMap.cpp	(revision 164365)
+++ lib/AST/ParentMap.cpp	(working copy)
@@ -21,12 +21,14 @@
 typedef llvm::DenseMap<Stmt*, Stmt*> MapTy;
 
 static void BuildParentMap(MapTy& M, Stmt* S) {
+  const PseudoObjectExpr* POE;
   for (Stmt::child_range I = S->children(); I; ++I)
     if (*I) {
-      // Prefer the first time we see this statement in the traversal.
-      // This is important for PseudoObjectExprs.
+      // For PseudoObjectExprs only the syntactic form is a parent.
+      // The second test ensures a proper updating of PseudoObjectExprs.
       Stmt *&Parent = M[*I];
-      if (!Parent) {
+      if ((POE = dyn_cast_or_null<PseudoObjectExpr>(Parent)) == 0 ||
+          POE->getSyntacticForm() == *I) {  
         Parent = S;
         BuildParentMap(M, *I);
       }