Volatile read checks and trivial constructor check fixed in r161393. On Mon, Aug 6, 2012 at 9:59 PM, Richard Smith <[email protected]> wrote:
> On Mon, Aug 6, 2012 at 9:46 PM, Eli Friedman <[email protected]>wrote: > >> On Mon, Aug 6, 2012 at 9:16 PM, Richard Smith >> <[email protected]> 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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
