[cfe-commits] r161388 - in /cfe/trunk: lib/AST/Expr.cpp lib/AST/ExprConstant.cpp test/CodeGenCXX/const-init-cxx11.cpp test/CodeGenCXX/debug-lambda-expressions.cpp test/CodeGenCXX/global-init.cpp test/CodeGenCXX/lambda-expressions.cpp test/CodeGen

[Richard Smith]

On Mon, Aug 6, 2012 at 9:46 PM, Eli Friedman <eli.friedman at gmail.com> wrote:

> On Mon, Aug 6, 2012 at 9:16 PM, Richard Smith
> <richard-llvm at metafoo.co.uk> wrote:
> > Author: rsmith
> > Date: Mon Aug  6 23:16:51 2012
> > New Revision: 161388
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=161388&view=rev
> > Log:
> > Teach Expr::HasSideEffects about all the Expr types, and fix a bug where
> it
> > was mistakenly classifying dynamic_casts which might throw as having no
> side
> > effects.
> >
> > Switch it from a visitor to a switch, so it is kept up-to-date as future
> Expr
> > nodes are added. Move it from ExprConstant.cpp to Expr.cpp, since it's
> not
> > really related to constant expression evaluation.
> >
> > Since we use HasSideEffect to determine whether to emit an unused global
> with
> > internal linkage, this has the effect of suppressing emission of globals
> in
> > some cases.
> >
> > I've left many of the Objective-C cases conservatively assuming that the
> > expression has side-effects. I'll leave it to someone with better
> knowledge
> > of Objective-C than mine to improve them.
> >
> > +  case DeclRefExprClass:
> > +  case ObjCIvarRefExprClass:
> > +    return getType().isVolatileQualified();
>
> Saying that a DeclRefExpr has a side-effect if it's volatile is weird
> and incomplete... an lvalue-to-rvalue conversion can have a
> side-effect, but the DeclRef itself doesn't.


I agree. This is inherited from the old HasSideEffects implementation. I
was intending to clean this up in a subsequent commit.


> > +  case CXXTypeidExprClass: {
> > +    // A typeid expression has side-effects if it can throw.
> > +    const CXXTypeidExpr *TE = cast<CXXTypeidExpr>(this);
> > +    if (TE->isTypeOperand())
> > +      return false;
> > +    const CXXRecordDecl *RD =
> > +        TE->getExprOperand()->getType()->getAsCXXRecordDecl();
> > +    if (!RD || !RD->isPolymorphic() ||
> > +        !TE->getExprOperand()->
> > +          Classify(const_cast<ASTContext&>(Ctx)).isGLValue())
> > +      // Not a glvalue of polymorphic class type: the expression is an
> > +      // unevaluated operand.
> > +      return false;
> > +    // Might throw.
> > +    return true;
> > +  }
>
> Do we really not have a helper routine for this?
>

I've not found one, but we clearly should. This is duplicated now at least
here, in Sema::BuildCXXTypeId and in CodeGenFunction::EmitCXXTypeidExpr.


> > +  case CXXConstructExprClass:
> > +  case CXXTemporaryObjectExprClass: {
> > +    const CXXConstructExpr *CE = cast<CXXConstructExpr>(this);
> > +    if (!CE->isElidable() && !CE->getConstructor()->isTrivial())
> > +      return true;
> > +    // An elidable or trivial constructor does not add any side-effects
> of its
> > +    // own. Just look at its arguments.
> > +    break;
> > +  }
>
> It's not obvious to me that an elidable constructor doesn't add
> side-effects; what exactly is HasSideEffects defined to return?


  /// HasSideEffects - This routine returns true for all those expressions
  /// which must be evaluated each time and must not be optimized away
  /// or evaluated at compile time. Example is a function call, volatile
  /// variable read.

... although some further investigation indicates that we have some callers
which want a stronger guarantee that evaluating the expression can't be
detected. I'll remove the elidable check and update the comment.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120806/9e1c5c07/attachment.html>