Improve the unused-value check to look into comma expressions and filter out

voids in sub-expressions.  Patch by Mike M!

Fixes PR4806.

llvm-svn: 98335

6 files changed, 83 insertions, 8 deletions

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index efd0fd1b8dc..1b3202dd428 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -788,6 +788,8 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1,
 
   switch (getStmtClass()) {
   default:
+    if (getType()->isVoidType())
+      return false;
     Loc = getExprLoc();
     R1 = getSourceRange();
     return true;
@@ -834,8 +836,8 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1,
         if (IE->getValue() == 0)
           return false;
 
-      return (BO->getRHS()->isUnusedResultAWarning(Loc, R1, R2, Ctx) ||
-              BO->getLHS()->isUnusedResultAWarning(Loc, R1, R2, Ctx));
+      return (BO->getLHS()->isUnusedResultAWarning(Loc, R1, R2, Ctx) ||
+              BO->getRHS()->isUnusedResultAWarning(Loc, R1, R2, Ctx));
     }
 
     if (BO->isAssignmentOp())
@@ -936,6 +938,8 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1,
       if (const Expr *E = dyn_cast<Expr>(CS->body_back()))
         return E->isUnusedResultAWarning(Loc, R1, R2, Ctx);
 
+    if (getType()->isVoidType())
+      return false;
     Loc = cast<StmtExpr>(this)->getLParenLoc();
     R1 = getSourceRange();
     return true;
@@ -949,6 +953,8 @@ bool Expr::isUnusedResultAWarning(SourceLocation &Loc, SourceRange &R1,
     R1 = cast<CStyleCastExpr>(this)->getSubExpr()->getSourceRange();
     return true;
   case CXXFunctionalCastExprClass: {
+    if (getType()->isVoidType())
+      return false;
     const CastExpr *CE = cast<CastExpr>(this);
     
     // If this is a cast to void or a constructor conversion, check the operand.
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 75d9f67ad9d..fd65c32c203 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -76,10 +76,6 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
   if (!E)
     return;
 
-  // Ignore expressions that have void type.
-  if (E->getType()->isVoidType())
-    return;
-
   SourceLocation Loc;
   SourceRange R1, R2;
   if (!E->isUnusedResultAWarning(Loc, R1, R2, Context))
@@ -103,6 +99,9 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
   }
       
   if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
+    if (E->getType()->isVoidType())
+      return;
+
     // If the callee has attribute pure, const, or warn_unused_result, warn with
     // a more specific message to make it clear what is happening.
     if (const Decl *FD = CE->getCalleeDecl()) {
diff --git a/clang/test/Sema/return.c b/clang/test/Sema/return.c
index 5d1c97f2a2e..3ab23f4f8f8 100644
--- a/clang/test/Sema/return.c
+++ b/clang/test/Sema/return.c
@@ -1,4 +1,4 @@
-// RUN: %clang %s -fsyntax-only -Xclang -verify -fblocks -Wno-unreachable-code
+// RUN: %clang %s -fsyntax-only -Xclang -verify -fblocks -Wno-unreachable-code -Wno-unused-value
 
 // clang emits the following warning by default.
 // With GCC, -pedantic, -Wreturn-type or -Wall are required to produce the 
diff --git a/clang/test/Sema/warn-unused-value.c b/clang/test/Sema/warn-unused-value.c
new file mode 100644
index 00000000000..2e0fa54effb
--- /dev/null
+++ b/clang/test/Sema/warn-unused-value.c
@@ -0,0 +1,53 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunused-value %s
+
+int i = 0;
+int j = 0;
+
+void foo();
+
+// PR4806
+void pr4806() {
+  1,foo();          // expected-warning {{expression result unused}}
+
+  // other
+  foo();
+  i;                // expected-warning {{expression result unused}}
+
+  i,foo();          // expected-warning {{expression result unused}}
+  foo(),i;          // expected-warning {{expression result unused}}
+
+  i,j,foo();        // expected-warning {{expression result unused}}
+  i,foo(),j;        // expected-warning {{expression result unused}}
+  foo(),i,j;        // expected-warning {{expression result unused}}
+
+  i++;
+
+  i++,foo();
+  foo(),i++;
+
+  i++,j,foo();      // expected-warning {{expression result unused}}
+  i++,foo(),j;      // expected-warning {{expression result unused}}
+  foo(),i++,j;      // expected-warning {{expression result unused}}
+
+  i,j++,foo();      // expected-warning {{expression result unused}}
+  i,foo(),j++;      // expected-warning {{expression result unused}}
+  foo(),i,j++;      // expected-warning {{expression result unused}}
+
+  i++,j++,foo();
+  i++,foo(),j++;
+  foo(),i++,j++;
+
+  {};
+  ({});
+  ({}),foo();
+  foo(),({});
+
+  (int)1U;          // expected-warning {{expression result unused}}
+  (void)1U;
+
+  // pointer to volatile has side effect (thus no warning)
+  int* pi = &i;
+  volatile int* pj = &j;
+  *pi;              // expected-warning {{expression result unused}}
+  *pj;
+}
diff --git a/clang/test/SemaCXX/decl-expr-ambiguity.cpp b/clang/test/SemaCXX/decl-expr-ambiguity.cpp
index 81f68392d85..4243c1c0ead 100644
--- a/clang/test/SemaCXX/decl-expr-ambiguity.cpp
+++ b/clang/test/SemaCXX/decl-expr-ambiguity.cpp
@@ -10,7 +10,7 @@ void f() {
   int(a)++; // expected-error {{expression is not assignable}}
   __extension__ int(a)++; // expected-error {{expression is not assignable}}
   __typeof(int)(a,5)<<a; // expected-error {{function-style cast to a builtin type can only take one argument}}
-  void(a), ++a; // expected-warning {{expression result unused}}
+  void(a), ++a;
   if (int(a)+1) {}
   for (int(a)+1;;) {} // expected-warning {{expression result unused}}
   a = sizeof(int()+1);
diff --git a/clang/test/SemaCXX/warn-unused-value.cpp b/clang/test/SemaCXX/warn-unused-value.cpp
new file mode 100644
index 00000000000..775c3cf01f2
--- /dev/null
+++ b/clang/test/SemaCXX/warn-unused-value.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunused-value %s
+
+// PR4806
+namespace test0 {
+  class Box {
+  public:
+    int i;
+    volatile int j;
+  };
+
+  void doit() {
+    // pointer to volatile has side effect (thus no warning)
+    Box* box = new Box;
+    box->i; // expected-warning {{expression result unused}}
+    box->j;
+  }
+}