On Jun 8, 2012, at 5:47 PM, Richard Trieu wrote:
> On Fri, Jun 8, 2012 at 4:29 PM, John McCall <[email protected]> wrote:
> On Jun 8, 2012, at 4:24 PM, Richard Trieu wrote:
> > Add a bit to VarDecl that is set to true when its address is taken or it
> > gets passed by non-const reference.
>
> Just out of curiosity, is this bit set for variables of class type that are
> used as an operand of a trivial copy constructor or copy assignment operator?
>
> John.
>
> No, the bit is not currently set for those uses.
Then please document the desired semantics more accurately, like "the address
is taken or bound to a reference other than either operand of a trivial copy
operation" or whatever semantics we actually settle on.
Please document that this is only accurate when all possible accesses have been
seen. For example, it is only accurate for local variables after their scope
ends, and it is only accurate for global variables at the end of the
translation unit and only if they have internal linkage. It is also basically
meaningless within templates.
Please document that this is only meant to be a conservative approximation of
whether the address is used; it may be true in situations when the address is
actually statically provable to not escape. Alas, your patch does not
implement a conservative approximation. Among other things, you're only
setting this bit in direct uses, but you really do need to be looking through
anything that propagates reference-ness, including member accesses,
base/derived casts, l-value reinterpreting casts, comma operators, and
conditional operators; I do not claim that this list is exhaustive. Other
things which take references to objects are lambda by-reference captures and
member calls on the object; this, too, I do not claim to be exhaustive.
That leads into the very interesting question of how we should validate the
correctness of this bit. We really can't rely on it until we have some sort of
validation story.
Arguably, this bit should never be set on a variable of reference type.
+ /// \brief Whether this variable has its address taken or is referenced.
+ unsigned HasReference : 1;
+ /// Whether this variable has its address taken or is referenced.
+ bool hasReference() const { return VarDeclBits.HasReference; }
+ void setReference(bool ref) { VarDeclBits.HasReference = ref; }
This is a poor choice of names; it is confusable with both the concept of a
variable of reference type and the concept of whether a declaration has been
referenced in the translation unit. I would go with isAddressTaken /
setAddressTaken, with the C++ behavior with references being understood (and
documented).
Also, the semantics you're actually tracking are quite a bit more subtle than
whether the address it taken, because you're specifically ignoring
initializations of const references. That's quite suspect and quite a bit less
generically useful. Also note that it is not illegal to bind a const reference
to something and then modify it through that reference, either due to a mutable
field or due to the constness being cast away.
setAddressTaken can default its argument to true.
You are not initializing this field to false.
You are not serializing and deserialization this field. The ParmVarDecl
abbreviation should assume this bit is 'false', I guess.
--- lib/Sema/SemaInit.cpp (revision 158223)
+++ lib/Sema/SemaInit.cpp (working copy)
@@ -6211,6 +6211,12 @@
Expr *InitE = Init.get();
assert(InitE && "No initialization expression?");
+ QualType QT = Entity.getType();
+ if (QT->isReferenceType() && !QT.getNonReferenceType().isConstQualified())
+ if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(InitE->IgnoreParens()))
+ if (VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl()))
+ VD->setReference(true);
This is not an appropriate place for this check; not everything goes through
PerformCopyInitialization. This needs to happen in
InitializationSequence::Perform for a reference-binding step.
+ if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(op))
+ if (VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl()))
+ VD->setReference(true);
You're not even looking through dyn_casts here. You should extract out a
function which walks an expression and marks variables that are bound in this
way. Alternatively, you may be able to piggy-back on the C++11 ODR-use code.
John.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
