<div class="gmail_quote">Volatile read checks and trivial constructor check fixed in r161393.</div><div class="gmail_quote"><br></div><div class="gmail_quote">On Mon, Aug 6, 2012 at 9:59 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div class="h5">On Mon, Aug 6, 2012 at 9:46 PM, Eli Friedman <span dir="ltr"><<a href="mailto:eli.friedman@gmail.com" target="_blank">eli.friedman@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Mon, Aug 6, 2012 at 9:16 PM, Richard Smith<br>
<<a href="mailto:richard-llvm@metafoo.co.uk" target="_blank">richard-llvm@metafoo.co.uk</a>> wrote:<br>
> Author: rsmith<br>
> Date: Mon Aug 6 23:16:51 2012<br>
> New Revision: 161388<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=161388&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=161388&view=rev</a><br>
> Log:<br>
> Teach Expr::HasSideEffects about all the Expr types, and fix a bug where it<br>
> was mistakenly classifying dynamic_casts which might throw as having no side<br>
> effects.<br>
><br>
> Switch it from a visitor to a switch, so it is kept up-to-date as future Expr<br>
> nodes are added. Move it from ExprConstant.cpp to Expr.cpp, since it's not<br>
> really related to constant expression evaluation.<br>
><br>
> Since we use HasSideEffect to determine whether to emit an unused global with<br>
> internal linkage, this has the effect of suppressing emission of globals in<br>
> some cases.<br>
><br>
> I've left many of the Objective-C cases conservatively assuming that the<br>
> expression has side-effects. I'll leave it to someone with better knowledge<br>
> of Objective-C than mine to improve them.<br>
><br>
> + case DeclRefExprClass:<br>
> + case ObjCIvarRefExprClass:<br>
> + return getType().isVolatileQualified();<br>
<br>
Saying that a DeclRefExpr has a side-effect if it's volatile is weird<br>
and incomplete... an lvalue-to-rvalue conversion can have a<br>
side-effect, but the DeclRef itself doesn't.</blockquote><div><br></div></div></div><div><div>I agree. This is inherited from the old HasSideEffects implementation. I was intending to clean this up in a subsequent commit.</div>
</div><div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + case CXXTypeidExprClass: {<br>
> + // A typeid expression has side-effects if it can throw.<br>
> + const CXXTypeidExpr *TE = cast<CXXTypeidExpr>(this);<br>
> + if (TE->isTypeOperand())<br>
> + return false;<br>
> + const CXXRecordDecl *RD =<br>
> + TE->getExprOperand()->getType()->getAsCXXRecordDecl();<br>
> + if (!RD || !RD->isPolymorphic() ||<br>
> + !TE->getExprOperand()-><br>
> + Classify(const_cast<ASTContext&>(Ctx)).isGLValue())<br>
> + // Not a glvalue of polymorphic class type: the expression is an<br>
> + // unevaluated operand.<br>
> + return false;<br>
> + // Might throw.<br>
> + return true;<br>
> + }<br>
<br>
Do we really not have a helper routine for this?<br></blockquote><div><br></div></div><div>I've not found one, but we clearly should. This is duplicated now at least here, in Sema::BuildCXXTypeId and in CodeGenFunction::EmitCXXTypeidExpr.</div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + case CXXConstructExprClass:<br>
> + case CXXTemporaryObjectExprClass: {<br>
> + const CXXConstructExpr *CE = cast<CXXConstructExpr>(this);<br>
> + if (!CE->isElidable() && !CE->getConstructor()->isTrivial())<br>
> + return true;<br>
> + // An elidable or trivial constructor does not add any side-effects of its<br>
> + // own. Just look at its arguments.<br>
> + break;<br>
> + }<br>
<br>
It's not obvious to me that an elidable constructor doesn't add<br>
side-effects; what exactly is HasSideEffects defined to return?</blockquote><div><br></div></div><div> /// HasSideEffects - This routine returns true for all those expressions</div><div> /// which must be evaluated each time and must not be optimized away</div>
<div> /// or evaluated at compile time. Example is a function call, volatile</div><div> /// variable read.</div><div><br></div></div>... 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.
</blockquote></div><br>