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
