[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]

Volatile read checks and trivial constructor check fixed in r161393.

On Mon, Aug 6, 2012 at 9:59 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> 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/ffbc79ed/attachment.html>