[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

Committed with minor modifications for the range-based for loop changes in 
r350404.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55955/new/

https://reviews.llvm.org/D55955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 3 inline comments as done.
aaron.ballman added inline comments.



Comment at: test/Parser/switch-recovery.cpp:108
   expected-error {{no member named 'x' in the global namespace; 
did you mean simply 'x'?}} \
-  expected-warning 2 {{expression result unused}}
+  expected-warning {{expression result unused}}
 9:: :y; // expected-error {{expected ';' after expression}} \

rsmith wrote:
> Hmm, why do we only get one warning here? I'd expect one warning for the `8;` 
> and one for the `x;` (after applying the fixes from the errors).
We get the one for the `8;` but the other one is a `TypoExpr` and it claims 
it's type dependent, and we don't warn on type dependent so we bail out pretty 
early in `Expr::isUnusedResultAWarning()`.



Comment at: test/SemaCXX/for-range-examples.cpp:181
 for (+x : {1, 2, 3}) {} // expected-error {{undeclared identifier}} 
expected-error {{expected ';'}}
-for (+y : {1, 2, 3}) {} // expected-error {{must declare a variable}}
+for (+y : {1, 2, 3}) {} // expected-error {{must declare a variable}} 
expected-warning {{expression result unused}}
   }

rsmith wrote:
> The new warnings here aren't ideal; do you know why they show up?
Because the first part in a for loop can be an expression, but only if it's not 
a range-based for loop. However, I was able to do some lookahead to retain the 
old behavior here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55955/new/

https://reviews.llvm.org/D55955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Thanks, this looks great. A couple of the changes to the tests look like the 
diagnostic output is slightly worse in some error recovery conditions, but 
generally this is a nice improvement.




Comment at: test/Parser/switch-recovery.cpp:108
   expected-error {{no member named 'x' in the global namespace; 
did you mean simply 'x'?}} \
-  expected-warning 2 {{expression result unused}}
+  expected-warning {{expression result unused}}
 9:: :y; // expected-error {{expected ';' after expression}} \

Hmm, why do we only get one warning here? I'd expect one warning for the `8;` 
and one for the `x;` (after applying the fixes from the errors).



Comment at: test/SemaCXX/for-range-examples.cpp:181
 for (+x : {1, 2, 3}) {} // expected-error {{undeclared identifier}} 
expected-error {{expected ';'}}
-for (+y : {1, 2, 3}) {} // expected-error {{must declare a variable}}
+for (+y : {1, 2, 3}) {} // expected-error {{must declare a variable}} 
expected-warning {{expression result unused}}
   }

The new warnings here aren't ideal; do you know why they show up?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55955/new/

https://reviews.llvm.org/D55955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 2 inline comments as done.
aaron.ballman added inline comments.



Comment at: include/clang/Sema/Sema.h:5337
   ExprResult ActOnFinishFullExpr(Expr *Expr, SourceLocation CC,
- bool DiscardedValue = false,
+ bool WarnOnDiscardedValue = false,
  bool IsConstexpr = false);

rsmith wrote:
> Quuxplusone wrote:
> > aaron.ballman wrote:
> > > rsmith wrote:
> > > > Why "WarnOn"? Shouldn't this flag simply indicate whether the 
> > > > expression is a discarded-value expression?
> > > It probably can; but then it feels like the logic is backwards from the 
> > > suggested changes as I understood them. If it's a discarded value 
> > > expression, then the value being unused should *not* be diagnosed because 
> > > the expression only exists for its side effects (not its value 
> > > computations), correct?
> > Peanut gallery says: There are at least three things that need to be 
> > computed somewhere: (1) Is this expression's value discarded? (2) Is this 
> > expression the result of a `[[nodiscard]]` function? (3) Is the diagnostic 
> > enabled? It is unclear to me who's responsible for computing which of these 
> > things. I.e., it is unclear to me whether `WarnOnDiscardedValue=true` means 
> > "Hey `ActOnFinishFullExpr`, please give a warning //because// this value is 
> > being discarded" (conjunction of 1,2, and maybe 3) or "Hey 
> > `ActOnFinishFullExpr`, please give a warning //if// this value is being 
> > discarded" (conjunction of 2 and maybe 3).
> > 
> > I also think it is needlessly confusing that `ActOnFinishFullExpr` gives 
> > `WarnOnDiscardedValue` a defaulted value of `false` but `ActOnExprStmt` 
> > gives `WarnOnDiscardedValue` a defaulted value of `true`. Defaulted values 
> > (especially of boolean type) are horrible, but context-dependent defaulted 
> > values are even worse.
> I don't think it makes sense for `ActOnFinishFullExpr` to have a default 
> argument for `DiscardedValue`, because there's really no reason to assume one 
> way or the other -- the values of some full-expressions are used, and the 
> values of others are not. A default of `false` certainly seems wrong.
> 
> For `ActOnExprStmt`, the default argument makes sense to me: the expression 
> in an expression-statement is by definition a discarded-value expression 
> (http://eel.is/c++draft/stmt.stmt#stmt.expr-1.sentence-2) -- it's only the 
> weird special case for a final expression-statement in an 
> statement-expression that bucks the trend here.
> 
> > If it's a discarded value expression, then the value being unused should 
> > *not* be diagnosed because the expression only exists for its side effects 
> > (not its value computations), correct?
> 
> No. If it's a discarded-value expression, that means the value of the 
> full-expression is not being used, so it should be diagnosed. If it's not a 
> discarded-value expression, then the value of the full-expression is used for 
> something (eg, it's a condition or an array bound or a template argument) and 
> so we should not warn. Indeed, the wording for `[[nodiscard]]` suggests to 
> warn (only) on potentially-evaluated discarded-value expressions.
> 
> Discarded-value expressions are things like expression-statements, the 
> left-hand-side of a comma operator, and the operands of casts to void. (Note 
> in the cast-to-void case is explicitly called out by the `[[nodiscard]]` 
> wording as a discarded-value expression that should not warn: 
> http://eel.is/c++draft/dcl.attr.nodiscard#2.sentence-2)
Thank you for the explanation @rsmith, my mental model was exactly backwards. 
:-D I was thrown off by 
http://eel.is/c++draft/expr.prop#expr.context-2.sentence-1 because I read it as 
saying the discarded nature of the results are *desired* rather than 
problematic. e.g., some statements only appear for their side effects, do not 
warn about such statements because the context makes it obvious that this is 
expected. I should have looked it it in the context of the nodiscard attribute 
wording rather than in isolation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55955/new/

https://reviews.llvm.org/D55955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 179920.
aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.

Updated based on review feedback. The vast majority of the changes are from the 
removal of a default argument to `ActOnFullExpr()`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55955/new/

https://reviews.llvm.org/D55955

Files:
  include/clang/Parse/Parser.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseObjc.cpp
  lib/Parse/ParseOpenMP.cpp
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaCoroutine.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaLambda.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaStmt.cpp
  lib/Sema/TreeTransform.h
  test/CXX/stmt.stmt/stmt.select/p3.cpp
  test/CodeCompletion/pragma-macro-token-caching.c
  test/Parser/cxx1z-init-statement.cpp
  test/Parser/switch-recovery.cpp
  test/SemaCXX/cxx1z-init-statement.cpp
  test/SemaCXX/for-range-examples.cpp
  test/SemaCXX/warn-unused-result.cpp
  test/SemaObjCXX/foreach.mm

Index: test/SemaObjCXX/foreach.mm
===
--- test/SemaObjCXX/foreach.mm
+++ test/SemaObjCXX/foreach.mm
@@ -6,8 +6,8 @@
 void f(NSArray *a) {
 id keys;
 for (int i : a); // expected-error{{selector element type 'int' is not a valid object}} 
-for ((id)2 : a);  // expected-error {{for range declaration must declare a variable}}
-for (2 : a); // expected-error {{for range declaration must declare a variable}}
+for ((id)2 : a);  // expected-error {{for range declaration must declare a variable}} expected-warning {{expression result unused}}
+for (2 : a); // expected-error {{for range declaration must declare a variable}} expected-warning {{expression result unused}}
   
   for (id thisKey : keys);
 
@@ -71,7 +71,7 @@
 @end
 void test2(NSObject *collection) {
   Test2 *obj;
-  for (obj.prop : collection) { // expected-error {{for range declaration must declare a variable}}
+  for (obj.prop : collection) { // expected-error {{for range declaration must declare a variable}} expected-warning {{property access result unused}}
   }
 }
 
Index: test/SemaCXX/warn-unused-result.cpp
===
--- test/SemaCXX/warn-unused-result.cpp
+++ test/SemaCXX/warn-unused-result.cpp
@@ -33,6 +33,36 @@
   const S &s4 = g1();
 }
 
+void testSubstmts(int i) {
+  switch (i) {
+  case 0:
+f(); // expected-warning {{ignoring return value}}
+  default:
+f(); // expected-warning {{ignoring return value}}
+  }
+
+  if (i)
+f(); // expected-warning {{ignoring return value}}
+  else
+f(); // expected-warning {{ignoring return value}}
+
+  while (i)
+f(); // expected-warning {{ignoring return value}}
+
+  do
+f(); // expected-warning {{ignoring return value}}
+  while (i);
+
+  for (f(); // expected-warning {{ignoring return value}}
+   ;
+   f() // expected-warning {{ignoring return value}}
+  )
+f(); // expected-warning {{ignoring return value}}
+
+  f(),  // expected-warning {{ignoring return value}}
+  (void)f();
+}
+
 struct X {
  int foo() __attribute__((warn_unused_result));
 };
@@ -206,3 +236,13 @@
   (void)++p;
 }
 } // namespace
+
+namespace PR39837 {
+[[clang::warn_unused_result]] int f(int);
+
+void g() {
+  int a[2];
+  for (int b : a)
+f(b); // expected-warning {{ignoring return value}}
+}
+} // namespace PR39837
Index: test/SemaCXX/for-range-examples.cpp
===
--- test/SemaCXX/for-range-examples.cpp
+++ test/SemaCXX/for-range-examples.cpp
@@ -176,9 +176,9 @@
 
 // Make sure these don't crash. Better diagnostics would be nice.
 for (: {1, 2, 3}) {} // expected-error {{expected expression}} expected-error {{expected ';'}}
-for (1 : {1, 2, 3}) {} // expected-error {{must declare a variable}}
+for (1 : {1, 2, 3}) {} // expected-error {{must declare a variable}} expected-warning {{expression result unused}}
 for (+x : {1, 2, 3}) {} // expected-error {{undeclared identifier}} expected-error {{expected ';'}}
-for (+y : {1, 2, 3}) {} // expected-error {{must declare a variable}}
+for (+y : {1, 2, 3}) {} // expected-error {{must declare a variable}} expected-warning {{expression result unused}}
   }
 }
 
@@ -244,7 +244,7 @@
 { 
   int b = 1, a[b];
   a[0] = 0;
-  [&] { for (int c : a) 0; } ();
+  [&] { for (int c : a) 0; } (); // expected-warning {{expression result unused}}
 }
 
 
Index: test/SemaCXX/cxx1z-init-statement.cpp
===
--- test/SemaCXX/cxx1z-init-statement.cpp
+++ test/SemaCXX/cxx1z-init-statement.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -std=c++1z -verify %s
-// RUN: %clang_cc1 -std=c++17 -verify %s
+// RUN: %clang_cc1 -std=c++1z -Wno-unused-value -verify %s
+// RUN: %clang_cc1 -std=c++17 -Wno-unused-value -verify %s
 
 void testIf() {
   int x = 0;
@@ -12,

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/Sema/Sema.h:5337
   ExprResult ActOnFinishFullExpr(Expr *Expr, SourceLocation CC,
- bool DiscardedValue = false,
+ bool WarnOnDiscardedValue = false,
  bool IsConstexpr = false);

Quuxplusone wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > Why "WarnOn"? Shouldn't this flag simply indicate whether the expression 
> > > is a discarded-value expression?
> > It probably can; but then it feels like the logic is backwards from the 
> > suggested changes as I understood them. If it's a discarded value 
> > expression, then the value being unused should *not* be diagnosed because 
> > the expression only exists for its side effects (not its value 
> > computations), correct?
> Peanut gallery says: There are at least three things that need to be computed 
> somewhere: (1) Is this expression's value discarded? (2) Is this expression 
> the result of a `[[nodiscard]]` function? (3) Is the diagnostic enabled? It 
> is unclear to me who's responsible for computing which of these things. I.e., 
> it is unclear to me whether `WarnOnDiscardedValue=true` means "Hey 
> `ActOnFinishFullExpr`, please give a warning //because// this value is being 
> discarded" (conjunction of 1,2, and maybe 3) or "Hey `ActOnFinishFullExpr`, 
> please give a warning //if// this value is being discarded" (conjunction of 2 
> and maybe 3).
> 
> I also think it is needlessly confusing that `ActOnFinishFullExpr` gives 
> `WarnOnDiscardedValue` a defaulted value of `false` but `ActOnExprStmt` gives 
> `WarnOnDiscardedValue` a defaulted value of `true`. Defaulted values 
> (especially of boolean type) are horrible, but context-dependent defaulted 
> values are even worse.
I don't think it makes sense for `ActOnFinishFullExpr` to have a default 
argument for `DiscardedValue`, because there's really no reason to assume one 
way or the other -- the values of some full-expressions are used, and the 
values of others are not. A default of `false` certainly seems wrong.

For `ActOnExprStmt`, the default argument makes sense to me: the expression in 
an expression-statement is by definition a discarded-value expression 
(http://eel.is/c++draft/stmt.stmt#stmt.expr-1.sentence-2) -- it's only the 
weird special case for a final expression-statement in an statement-expression 
that bucks the trend here.

> If it's a discarded value expression, then the value being unused should 
> *not* be diagnosed because the expression only exists for its side effects 
> (not its value computations), correct?

No. If it's a discarded-value expression, that means the value of the 
full-expression is not being used, so it should be diagnosed. If it's not a 
discarded-value expression, then the value of the full-expression is used for 
something (eg, it's a condition or an array bound or a template argument) and 
so we should not warn. Indeed, the wording for `[[nodiscard]]` suggests to warn 
(only) on potentially-evaluated discarded-value expressions.

Discarded-value expressions are things like expression-statements, the 
left-hand-side of a comma operator, and the operands of casts to void. (Note in 
the cast-to-void case is explicitly called out by the `[[nodiscard]]` wording 
as a discarded-value expression that should not warn: 
http://eel.is/c++draft/dcl.attr.nodiscard#2.sentence-2)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55955/new/

https://reviews.llvm.org/D55955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: include/clang/Sema/Sema.h:5337
   ExprResult ActOnFinishFullExpr(Expr *Expr, SourceLocation CC,
- bool DiscardedValue = false,
+ bool WarnOnDiscardedValue = false,
  bool IsConstexpr = false);

aaron.ballman wrote:
> rsmith wrote:
> > Why "WarnOn"? Shouldn't this flag simply indicate whether the expression is 
> > a discarded-value expression?
> It probably can; but then it feels like the logic is backwards from the 
> suggested changes as I understood them. If it's a discarded value expression, 
> then the value being unused should *not* be diagnosed because the expression 
> only exists for its side effects (not its value computations), correct?
Peanut gallery says: There are at least three things that need to be computed 
somewhere: (1) Is this expression's value discarded? (2) Is this expression the 
result of a `[[nodiscard]]` function? (3) Is the diagnostic enabled? It is 
unclear to me who's responsible for computing which of these things. I.e., it 
is unclear to me whether `WarnOnDiscardedValue=true` means "Hey 
`ActOnFinishFullExpr`, please give a warning //because// this value is being 
discarded" (conjunction of 1,2, and maybe 3) or "Hey `ActOnFinishFullExpr`, 
please give a warning //if// this value is being discarded" (conjunction of 2 
and maybe 3).

I also think it is needlessly confusing that `ActOnFinishFullExpr` gives 
`WarnOnDiscardedValue` a defaulted value of `false` but `ActOnExprStmt` gives 
`WarnOnDiscardedValue` a defaulted value of `true`. Defaulted values 
(especially of boolean type) are horrible, but context-dependent defaulted 
values are even worse.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55955/new/

https://reviews.llvm.org/D55955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 3 inline comments as done.
aaron.ballman added inline comments.



Comment at: include/clang/Sema/Sema.h:5337
   ExprResult ActOnFinishFullExpr(Expr *Expr, SourceLocation CC,
- bool DiscardedValue = false,
+ bool WarnOnDiscardedValue = false,
  bool IsConstexpr = false);

rsmith wrote:
> Why "WarnOn"? Shouldn't this flag simply indicate whether the expression is a 
> discarded-value expression?
It probably can; but then it feels like the logic is backwards from the 
suggested changes as I understood them. If it's a discarded value expression, 
then the value being unused should *not* be diagnosed because the expression 
only exists for its side effects (not its value computations), correct?



Comment at: lib/Parse/ParseStmt.cpp:23
 #include "clang/Sema/Scope.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/TypoCorrection.h"

rsmith wrote:
> The parser shouldn't be looking inside Sema's data structures. Can you add a 
> method to the Sema interface for the query you make below instead?
Can do.



Comment at: lib/Sema/TreeTransform.h:6529
+  --SuppressWarnOnUnusedExpr;
+
 if (Result.isInvalid()) {

rsmith wrote:
> Instead of this counter mechanism (which doesn't seem reliable), can you just 
> pass a flag to TransformStmt?
I'll give it a shot, it seems plausible.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55955/new/

https://reviews.llvm.org/D55955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/Sema/Sema.h:5337
   ExprResult ActOnFinishFullExpr(Expr *Expr, SourceLocation CC,
- bool DiscardedValue = false,
+ bool WarnOnDiscardedValue = false,
  bool IsConstexpr = false);

Why "WarnOn"? Shouldn't this flag simply indicate whether the expression is a 
discarded-value expression?



Comment at: lib/Parse/ParseStmt.cpp:23
 #include "clang/Sema/Scope.h"
+#include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/TypoCorrection.h"

The parser shouldn't be looking inside Sema's data structures. Can you add a 
method to the Sema interface for the query you make below instead?



Comment at: lib/Sema/SemaExprCXX.cpp:7785
 
-  if (DiscardedValue) {
+  if (WarnOnDiscardedValue) {
 // Top-level expressions default to 'id' when we're in a debugger.

It doesn't make sense for a WarnOn flag to control how we build the AST like 
this.



Comment at: lib/Sema/TreeTransform.h:3297
 
-  return getSema().ActOnExprStmt(E);
+  return getSema().ActOnExprStmt(E, SuppressWarnOnUnusedExpr == 0);
 }

What happens if the last statement in an expression statement contains a 
lambda? Will we consider all expression statements in it as not being 
discarded-value expressions? Alternate suggestion below.



Comment at: lib/Sema/TreeTransform.h:6529
+  --SuppressWarnOnUnusedExpr;
+
 if (Result.isInvalid()) {

Instead of this counter mechanism (which doesn't seem reliable), can you just 
pass a flag to TransformStmt?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55955/new/

https://reviews.llvm.org/D55955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 179475.
aaron.ballman marked 3 inline comments as done.
aaron.ballman added a comment.

Updated based on review feedback.

The lookahead wasn't too annoying to thread through, but I did run into 
surprises in TreeTransform where I can't use the lookahead to determine whether 
the expression statement should be warned on or not. The solution I have is 
serviceable, but perhaps there's a better approach to be taken.

The changes introduce warnings where they were previously missed, such as in 
init-statements. The only unfortunate behavioral side effect comes from 
range-based for loops that have an expression as the first statement -- in 
addition to the error, we now trigger an unused expression result warning, but 
restructuring the code to avoid it would be challenging for such an edge case 
(and the warning behavior seems reasonable).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55955/new/

https://reviews.llvm.org/D55955

Files:
  include/clang/Parse/Parser.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseObjc.cpp
  lib/Parse/ParseOpenMP.cpp
  lib/Parse/ParseStmt.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaOpenMP.cpp
  lib/Sema/SemaStmt.cpp
  lib/Sema/TreeTransform.h
  test/CXX/stmt.stmt/stmt.select/p3.cpp
  test/CodeCompletion/pragma-macro-token-caching.c
  test/Parser/cxx1z-init-statement.cpp
  test/Parser/switch-recovery.cpp
  test/SemaCXX/cxx1z-init-statement.cpp
  test/SemaCXX/for-range-examples.cpp
  test/SemaCXX/warn-unused-result.cpp
  test/SemaObjCXX/foreach.mm

Index: test/SemaObjCXX/foreach.mm
===
--- test/SemaObjCXX/foreach.mm
+++ test/SemaObjCXX/foreach.mm
@@ -6,8 +6,8 @@
 void f(NSArray *a) {
 id keys;
 for (int i : a); // expected-error{{selector element type 'int' is not a valid object}} 
-for ((id)2 : a);  // expected-error {{for range declaration must declare a variable}}
-for (2 : a); // expected-error {{for range declaration must declare a variable}}
+for ((id)2 : a);  // expected-error {{for range declaration must declare a variable}} expected-warning {{expression result unused}}
+for (2 : a); // expected-error {{for range declaration must declare a variable}} expected-warning {{expression result unused}}
   
   for (id thisKey : keys);
 
@@ -71,7 +71,7 @@
 @end
 void test2(NSObject *collection) {
   Test2 *obj;
-  for (obj.prop : collection) { // expected-error {{for range declaration must declare a variable}}
+  for (obj.prop : collection) { // expected-error {{for range declaration must declare a variable}} expected-warning {{property access result unused}}
   }
 }
 
Index: test/SemaCXX/warn-unused-result.cpp
===
--- test/SemaCXX/warn-unused-result.cpp
+++ test/SemaCXX/warn-unused-result.cpp
@@ -33,6 +33,36 @@
   const S &s4 = g1();
 }
 
+void testSubstmts(int i) {
+  switch (i) {
+  case 0:
+f(); // expected-warning {{ignoring return value}}
+  default:
+f(); // expected-warning {{ignoring return value}}
+  }
+
+  if (i)
+f(); // expected-warning {{ignoring return value}}
+  else
+f(); // expected-warning {{ignoring return value}}
+
+  while (i)
+f(); // expected-warning {{ignoring return value}}
+
+  do
+f(); // expected-warning {{ignoring return value}}
+  while (i);
+
+  for (f(); // expected-warning {{ignoring return value}}
+   ;
+   f() // expected-warning {{ignoring return value}}
+  )
+f(); // expected-warning {{ignoring return value}}
+
+  f(),  // expected-warning {{ignoring return value}}
+  (void)f();
+}
+
 struct X {
  int foo() __attribute__((warn_unused_result));
 };
@@ -206,3 +236,13 @@
   (void)++p;
 }
 } // namespace
+
+namespace PR39837 {
+[[clang::warn_unused_result]] int f(int);
+
+void g() {
+  int a[2];
+  for (int b : a)
+f(b); // expected-warning {{ignoring return value}}
+}
+} // namespace PR39837
Index: test/SemaCXX/for-range-examples.cpp
===
--- test/SemaCXX/for-range-examples.cpp
+++ test/SemaCXX/for-range-examples.cpp
@@ -176,9 +176,9 @@
 
 // Make sure these don't crash. Better diagnostics would be nice.
 for (: {1, 2, 3}) {} // expected-error {{expected expression}} expected-error {{expected ';'}}
-for (1 : {1, 2, 3}) {} // expected-error {{must declare a variable}}
+for (1 : {1, 2, 3}) {} // expected-error {{must declare a variable}} expected-warning {{expression result unused}}
 for (+x : {1, 2, 3}) {} // expected-error {{undeclared identifier}} expected-error {{expected ';'}}
-for (+y : {1, 2, 3}) {} // expected-error {{must declare a variable}}
+for (+y : {1, 2, 3}) {} // expected-error {{must declare a variable}} expected-warning {{expression result unused}}
   }
 }
 
@@ -244,7 +244,7 @@
 { 
   int b = 1, a[b];
   a[0] = 0;
-  [&] { for (int c : a) 0; } ();
+  [&] { for (int c : a) 0; } (); // expected-wa

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Sema/SemaStmt.cpp:2846
 diag::warn_empty_range_based_for_body);
+  DiagnoseUnusedExprResult(B);
 

aaron.ballman wrote:
> rsmith wrote:
> > rsmith wrote:
> > > While this looks correct per the current approach to this function, we 
> > > really shouldn't be duplicating calls to this everywhere. Can we move all 
> > > the calls to a single call in `ActOnFinishFullStmt`?
> > Looks like that's not actually called from almost anywhere, but checking 
> > from `ActOnFinishFullExpr` in the case where `DiscardedValue` is `true` 
> > seems appropriate.
> That seems sensible, but how will that work with GNU statement expressions? 
> If we check for unused expression results, then we will trigger on code like 
> this:
> ```
> int a = ({blah(); yada(); 0;});
> // or
> int b = ({blah(); yada(); some_no_discard_call();});
> ```
> I don't know of a way to handle that case -- we call `ActOnFinishFullExpr()` 
> while parsing the compound statement for the `StmtExpr`, so there's no way to 
> know whether there's another statement coming (without lookahead) to 
> determine whether to suppress the diagnostic for the last expression 
> statement. Do you have ideas on how to handle that?
If we're calling `ActOnFinishFullExpr` there with `DiscardedValue == true`, 
that would be a bug that we should be fixing regardless. I don't think the 
lookahead is so bad itself -- it should just be a one-token lookahead for a `}` 
after the `;` -- but it might be awkward to wire it into our compound-statement 
/ expression-statement parsing. Still, it seems necessary for correctness.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55955/new/

https://reviews.llvm.org/D55955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 3 inline comments as done.
aaron.ballman added inline comments.



Comment at: lib/Sema/SemaStmt.cpp:2846
 diag::warn_empty_range_based_for_body);
+  DiagnoseUnusedExprResult(B);
 

rsmith wrote:
> rsmith wrote:
> > While this looks correct per the current approach to this function, we 
> > really shouldn't be duplicating calls to this everywhere. Can we move all 
> > the calls to a single call in `ActOnFinishFullStmt`?
> Looks like that's not actually called from almost anywhere, but checking from 
> `ActOnFinishFullExpr` in the case where `DiscardedValue` is `true` seems 
> appropriate.
That seems sensible, but how will that work with GNU statement expressions? If 
we check for unused expression results, then we will trigger on code like this:
```
int a = ({blah(); yada(); 0;});
// or
int b = ({blah(); yada(); some_no_discard_call();});
```
I don't know of a way to handle that case -- we call `ActOnFinishFullExpr()` 
while parsing the compound statement for the `StmtExpr`, so there's no way to 
know whether there's another statement coming (without lookahead) to determine 
whether to suppress the diagnostic for the last expression statement. Do you 
have ideas on how to handle that?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55955/new/

https://reviews.llvm.org/D55955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Sema/SemaStmt.cpp:2846
 diag::warn_empty_range_based_for_body);
+  DiagnoseUnusedExprResult(B);
 

rsmith wrote:
> While this looks correct per the current approach to this function, we really 
> shouldn't be duplicating calls to this everywhere. Can we move all the calls 
> to a single call in `ActOnFinishFullStmt`?
Looks like that's not actually called from almost anywhere, but checking from 
`ActOnFinishFullExpr` in the case where `DiscardedValue` is `true` seems 
appropriate.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55955/new/

https://reviews.llvm.org/D55955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Sema/SemaStmt.cpp:2846
 diag::warn_empty_range_based_for_body);
+  DiagnoseUnusedExprResult(B);
 

While this looks correct per the current approach to this function, we really 
shouldn't be duplicating calls to this everywhere. Can we move all the calls to 
a single call in `ActOnFinishFullStmt`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55955/new/

https://reviews.llvm.org/D55955



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55955: Properly diagnose [[nodiscard]] on the body of a range-based for loop

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: rsmith, Quuxplusone, erik.pilkington.

If the range-based for loop function body is not a compound statement, we would 
fail to diagnose discarded results from the statement. This only impacted 
range-based for loops because other kinds of for loops already manually check 
the body for unused expression results.

This addresses PR39837.


https://reviews.llvm.org/D55955

Files:
  lib/Sema/SemaStmt.cpp
  test/SemaCXX/warn-unused-result.cpp


Index: test/SemaCXX/warn-unused-result.cpp
===
--- test/SemaCXX/warn-unused-result.cpp
+++ test/SemaCXX/warn-unused-result.cpp
@@ -206,3 +206,13 @@
   (void)++p;
 }
 } // namespace
+
+namespace PR39837 {
+[[clang::warn_unused_result]] int f(int);
+
+void g() {
+  int a[2];
+  for (int b : a)
+f(b); // expected-warning {{ignoring return value}}
+}
+} // namespace PR39837
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -2843,6 +2843,7 @@
 
   DiagnoseEmptyStmtBody(ForStmt->getRParenLoc(), B,
 diag::warn_empty_range_based_for_body);
+  DiagnoseUnusedExprResult(B);
 
   DiagnoseForRangeVariableCopies(*this, ForStmt);
 


Index: test/SemaCXX/warn-unused-result.cpp
===
--- test/SemaCXX/warn-unused-result.cpp
+++ test/SemaCXX/warn-unused-result.cpp
@@ -206,3 +206,13 @@
   (void)++p;
 }
 } // namespace
+
+namespace PR39837 {
+[[clang::warn_unused_result]] int f(int);
+
+void g() {
+  int a[2];
+  for (int b : a)
+f(b); // expected-warning {{ignoring return value}}
+}
+} // namespace PR39837
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -2843,6 +2843,7 @@
 
   DiagnoseEmptyStmtBody(ForStmt->getRParenLoc(), B,
 diag::warn_empty_range_based_for_body);
+  DiagnoseUnusedExprResult(B);
 
   DiagnoseForRangeVariableCopies(*this, ForStmt);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits