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