[PATCH] some StmtExprs do not have side-effects

[Aaron Ballman]

On Wed, Jun 3, 2015 at 6:45 AM, scott douglass <sdouglass at arm.com> wrote:
> Refine Expr::HasSideEffects to correct identify StmtExprs that do not have side-effects.
>
> [Depends on http://reviews.llvm.org/D10210.]
>
> http://reviews.llvm.org/D10211
>
> Files:
>   include/clang/AST/EvaluatedExprVisitor.h
>   lib/AST/Expr.cpp
>   test/Sema/exprs.c
>
> Index: include/clang/AST/EvaluatedExprVisitor.h
> ===================================================================
> --- include/clang/AST/EvaluatedExprVisitor.h
> +++ include/clang/AST/EvaluatedExprVisitor.h
> @@ -28,6 +28,7 @@
>  /// 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:
> Index: lib/AST/Expr.cpp
> ===================================================================
> --- lib/AST/Expr.cpp
> +++ lib/AST/Expr.cpp
> @@ -2881,6 +2881,32 @@
>    return false;
>  }
>
> +namespace {
> +  /// \brief Look for any side effects within a Stmt.
> +  class SideEffectFinder : public ConstEvaluatedExprVisitor<SideEffectFinder>
> +  {

Move curly brace up.

> +    typedef ConstEvaluatedExprVisitor<SideEffectFinder> Inherited;
> +    const bool IncludePossibleEffects;
> +    bool HasSideEffects;
> +
> +  public:
> +    explicit SideEffectFinder(const ASTContext &Context, bool IncludePossibleEffects_)

No trailing underscore in the param name, please.

> +      : Inherited(Context),
> +        IncludePossibleEffects(IncludePossibleEffects_), HasSideEffects(false) { }
> +
> +    bool getHasSideEffects() const { return HasSideEffects; }

can drop the get and just name it hasSideEffects().

> +
> +    void VisitExpr(const Expr *E) {
> +      if (!E->HasSideEffects(Context, IncludePossibleEffects)) {
> +        Inherited::VisitStmt(E);
> +        return;
> +      }
> +
> +      HasSideEffects = true;
> +    }
> +  };
> +}
> +
>  bool Expr::HasSideEffects(const ASTContext &Ctx,
>                            bool IncludePossibleEffects) const {
>    // In circumstances where we care about definite side effects instead of
> @@ -2967,14 +2993,20 @@
>    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-efect if any substatement does.

Typo: side-efect

> +    SideEffectFinder Finder(Ctx, IncludePossibleEffects);
> +    Finder.Visit(cast<StmtExpr>(this)->getSubStmt());
> +    return Finder.getHasSideEffects();
> +  }
> +
>    case ParenExprClass:
>    case ArraySubscriptExprClass:
>    case MemberExprClass:
> Index: test/Sema/exprs.c
> ===================================================================
> --- test/Sema/exprs.c
> +++ test/Sema/exprs.c
> @@ -251,3 +251,14 @@
>    if (&test22)
>      (void) 0;
>  }
> +
> +int stmtexpr_fn();
> +void stmtexprs(int i) {
> +  // No "side effects" warnig for the first two:

Typo: warnig

> +  __builtin_assume( ({ 1; }) ); // expected-warning {{use of GNU statement expression extension}}
> +  __builtin_assume( ({ if (i) { (void)0; }; 42; }) ); // expected-warning {{use of GNU statement expression extension}}
> +  __builtin_assume( ({ if (i) ({ // expected-warning {{use of GNU statement expression extension}} \
> +                                 // expected-warning {{use of GNU statement expression extension}} \
> +                                 // expected-warning {{the argument to '__builtin_assume' has side effects that will be discarded}}
> +                                 stmtexpr_fn(); }); 1; }) );

These warnings being inline make the test really hard to follow along
with. Consider using expected-warning at +1 instead. It may also make
sense to silence the statement expression warnings (perhaps that
requires moving the test to a different file) as those detract from
readability.

~Aaron

> +}
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>