[cfe-commits] [PATCH] Model C++11 rules for odr-use for variables correctly

[John McCall]

On Jan 31, 2012, at 9:43 PM, Eli Friedman wrote:
> New version attached; similar approach, but avoiding the recursive
> walk by keeping the relevant expressions in a side-table.  This is
> essentially complete except for one issue: I think I need some sort of
> stack to prevent incorrect odr-use marking in some edge cases.

Clever idea!  I think this works.

I believe the only construct that allows nested full-expressions is a
statement-expression.  You should optimize for the case that doesn't
involve statement-expressions, e.g. by just moving the current set
aside whenever we enter one.  I think we might even have a RAII
object for doing things like this already.

+void Sema::UpdateMarkingForLValueToRValue(Expr *E) {
+  MaybeODRUseExprs.erase(E->IgnoreParens());
+}

Hmm.  It might be useful to stash in the expression that it's being used
this way.  That might not play well with expression re-use, though.
No changes suggested. :)

(leaving that comment in place for the evidence trail)

 /// \brief Build a MemberExpr AST node.
-static MemberExpr *BuildMemberExpr(ASTContext &C, Expr *Base, bool isArrow,
+static MemberExpr *BuildMemberExpr(Sema &SemaRef,
+                                   ASTContext &C, Expr *Base, bool isArrow,

If you're passing in Sema, you don't also need to pass in the ASTContext.

+bool IsPotentiallyEvaluatedContext(Sema &SemaRef) {
...
+void DoMarkVarDeclReferenced(Sema &SemaRef, SourceLocation Loc,
+                             VarDecl *Var, Expr *E) {

These are missing 'static'.  Also, the convention for functions is to start with
a lowercase letter;  we haven't been doing mass-changes, but if you're
touching every call site anyway, please do follow the convention here.

-/// \brief Note that the given declaration was referenced in the source code.
-///
-/// This routine should be invoke whenever a given declaration is referenced
-/// in the source code, and where that reference occurred. If this declaration
-/// reference means that the the declaration is used (C++ [basic.def.odr]p2,
-/// C99 6.9p3), then the declaration will be marked as used.
-///
-/// \param Loc the location where the declaration was referenced.
-///
-/// \param D the declaration that has been referenced by the source code.

Killing this comment is fine, but please add comments to the new functions,
and make sure one of them conveys this whole scheme appropriately and
why it matters.  Make sure that the ones that don't take Exprs clearly spell
out the semantic difference.

Also, please split the restructure of MarkDeclarationReferenced into
declaration-specific helpers into a separate "no functionality change" patch;
otherwise it looks like you're significantly revising the logic there, when in
fact you're mostly just re-indenting it.

John.