[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