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

Jordan Rose jordan_rose at apple.com
Mon Sep 24 10:49:05 PDT 2012

On Sep 24, 2012, at 2:13 , Olaf Krzikalla <Olaf.Krzikalla at tu-dresden.de> wrote:

> Hi Jordan,
> Am 21.09.2012 18:18, schrieb Jordan Rose:
>> Interesting, and sorry for the comment-incompatible update. Let me explain my rationale for that change.
>> OpaqueValueExpr is intended to represent a value that is computed once but used in multiple places; the simplest example I've seen is for BinaryConditionalOperator (a GNU extension):
>> int *x = y ?: z;
>> In this expression, the condition of the operator is an ImplicitCastExpr (PointerToBool) around a DeclRefExpr (y), and the true-result is just the DeclRefExpr.* We don't want to compute the same value twice because of side effects, and nor do we want to traverse the source expression twice with a RecursiveASTVisitor, so we use OpaqueValueExpr to represent the evaluation of 'y'. In this particular case, it's not so clear whether the "canonical" parent of the DeclRefExpr is the ImplicitCastExpr or the BinaryConditionalOperator.
>> * There is more going on here (LValueToRValue conversion, for one thing), but I'm doing this from memory and trying to keep it simple.
> I'm concerned about instances in the AST with mutliple parents. I know that clang's AST represents more than just a literal AST. However I wouldn't go so far and allow multiple parents in the AST. That said, the IMHO correct solution is to remove getCond and getTrueExpr from AbstractConditionalOperator and handle the LHS of BinaryConditionalOperator differently. But according to the comments I don't think that my problem stems from OpaqueValueExpr.

"Expressions with multiple parents" is exactly the problem that OpaqueValueExpr is trying to solve. When you traverse the AST, the "source expr" of an OpaqueValueExpr is not visited -- you have to explicitly request that. OpaqueValueExpr is a small way to do AST canonicalization without breaking invariants or losing source information.

>> 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. An Objective-C property reference gets rebuilt as a PseudoObjectExpr in Sema. The analyzer doesn't have to do anything special to handle it because the CFG builder knows to just use the "semantic" representation, but when we go to report diagnostics, we'd like to know if a given Objective-C message was written as a message send or a property/subscript access. The specific case of ParentMap is to ensure that Xcode's arrows draw correctly even when highlighting something inside a PseudoObjectExpr—if you revert the change, you'll notice retain-release-path-notes.m starts failing in FileCheck because the plist serialization doesn't match.

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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120924/b106f6e3/attachment.html>

More information about the cfe-dev mailing list