some StmtExprs do not have side-effects

[scott-0]

Summary

Refine Expr::HasSideEffects to correct identify StmtExprs that do not have side-effects.

[Depends on http://reviews.llvm.org/D10210.]

[scott-0]

I see two typos in comments already "side-efect" and "warnig" -- I'll be sure to fix those.

[rs]

Just one small comment.

lib/AST/Expr.cpp

2904 ↗

(On Diff #27032)

Remove newline

[rs]

LGTM, but someone else should approve as well

[rsmith]

lib/AST/Expr.cpp

2886–2887 ↗

(On Diff #27032)

{ goes on previous line.

2901 ↗

(On Diff #27032)

Remove this call: you've already completely checked E and all of its subexpressions for side-effects. Calling into the base class here will result in a (worst-case) exponential-time search as both VisitStmt and HasSideEffects will recurse into subexpressions at every level.

[scott-0]

Thanks for the reviews! I have addressed the review comments. (But I still need a review on the prereq differential http://reviews.llvm.org/D10210)

[rsmith]

LGTM

lib/AST/Expr.cpp

2899–2900 ↗

(On Diff #27202)

Maybe only call E->HasSideEffect(...) if HasSideEffect is false. There's no point doing more checking once we've already found a side-effect.

cfe/trunk/include/clang/AST/EvaluatedExprVisitor.h

 namespace clang {
   
 class ASTContext;
   
 /// \brief Given a potentially-evaluated expression, this visitor visits all
 /// of its potentially-evaluated subexpressions, recursively.
 template<template <typename> class Ptr, typename ImplClass>
 class EvaluatedExprVisitorBase : public StmtVisitorBase<Ptr, ImplClass, void> {
+protected:
   const ASTContext &Context;
 
 public:
 #define PTR(CLASS) typename Ptr<CLASS>::type
 
   explicit EvaluatedExprVisitorBase(const ASTContext &Context) : Context(Context) { }
 
   // Expressions that have no potentially-evaluated subexpressions (but may have

cfe/trunk/lib/AST/Expr.cpp

bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef,
   }
   if (isEvaluatable(Ctx))
     return true;
   if (Culprit)
     *Culprit = this;
   return false;
 }
 
+namespace {
+  /// \brief Look for any side effects within a Stmt.
+  class SideEffectFinder : public ConstEvaluatedExprVisitor<SideEffectFinder> {
+    typedef ConstEvaluatedExprVisitor<SideEffectFinder> Inherited;
+    const bool IncludePossibleEffects;
+    bool HasSideEffects;
+
+  public:
+    explicit SideEffectFinder(const ASTContext &Context, bool IncludePossible)
+      : Inherited(Context),
+        IncludePossibleEffects(IncludePossible), HasSideEffects(false) { }
+
+    bool hasSideEffects() const { return HasSideEffects; }
+
+    void VisitExpr(const Expr *E) {
+      if (!HasSideEffects &&
+          E->HasSideEffects(Context, IncludePossibleEffects))
+        HasSideEffects = true;
+    }
+  };
+}
+
 bool Expr::HasSideEffects(const ASTContext &Ctx,
                           bool IncludePossibleEffects) const {
   // In circumstances where we care about definite side effects instead of
   // potential side effects, we want to ignore expressions that are part of a
   // macro expansion as a potential side effect.
   if (!IncludePossibleEffects && getExprLoc().isMacroID())
     return false;
 
case CXXBindTemporaryExprClass:
     if (!IncludePossibleEffects)
       break;
     return true;
 
   case MSPropertyRefExprClass:
   case CompoundAssignOperatorClass:
   case VAArgExprClass:
   case AtomicExprClass:
-  case StmtExprClass:
   case CXXThrowExprClass:
   case CXXNewExprClass:
   case CXXDeleteExprClass:
   case ExprWithCleanupsClass:
     // These always have a side-effect.
     return true;
 
+  case StmtExprClass: {
+    // StmtExprs have a side-effect if any substatement does.
+    SideEffectFinder Finder(Ctx, IncludePossibleEffects);
+    Finder.Visit(cast<StmtExpr>(this)->getSubStmt());
+    return Finder.hasSideEffects();
+  }
+
   case ParenExprClass:
   case ArraySubscriptExprClass:
   case MemberExprClass:
   case ConditionalOperatorClass:
   case BinaryConditionalOperatorClass:
   case CompoundLiteralExprClass:
   case ExtVectorElementExprClass:
   case DesignatedInitExprClass:

cfe/trunk/test/Sema/stmtexprs.c

+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -Wno-gnu-statement-expression
+
+int stmtexpr_fn();
+void stmtexprs(int i) {
+  __builtin_assume( ({ 1; }) ); // no warning about "side effects"
+  __builtin_assume( ({ if (i) { (void)0; }; 42; }) ); // no warning about "side effects"
+  // expected-warning@+1 {{the argument to '__builtin_assume' has side effects that will be discarded}}
+  __builtin_assume( ({ if (i) ({ stmtexpr_fn(); }); 1; }) );
+}