[libcxx-commits] [PATCH] D155064: [clang][SemaCXX] Diagnose tautological uses of consteval if and is_constant_evaluated

[Takuya Shimizu via Phabricator via libcxx-commits]

hazohelet added a comment.

I've noticed several shortcoming/limitations of this patch.

1. Function arguments: When we parse the arguments to a function call, the callee isn't still resolved, so we don't know whether it's consteval. If the callee is consteval, its argument is known to be constant-evaluated regardless of its surrounding evaluation context.

e.g.

  consteval int ce(int n){ return n;};
  int f() {
  	int a = ce(__builtin_is_constant_evaluated()); // tautologically true
  }

2. init-statement of constexpr-if:

The condition is required to be a constant expression, but the init-stmt before it isn't.
e.g. Given `if constexpr (InitStmt; Cond) {}`, `InitStmt` is evaluated in the surrounding evaluation context.
If the init-statement is a declaration we can push initializer evaluation context like we do in other variables. However, if the init-statement is an expression, when clang parses the expression, clang doesn't know whether it is parsing the init-statement or the condition. https://github.com/llvm/llvm-project/blob/e3c57fdd8439ba82c67347629a3c66f293e1f3d0/clang/lib/Parse/ParseExprCXX.cpp#L2102

If we are to handle these cases with perfection, we need to defer warning emissions, but I'm skeptical about the value we obtain from this effort.

For the first problem, we can make `IsRuntimeEvaluated` false while parsing arguments to avoid emitting incorrect diagnostics although it makes false negatives on tautologically-false uses in arguments.
The second problem is difficult because we cannot avoid incorrect diagnostics in `InitStmt` expression of constexpr-if with a simple fix. But I think it's acceptable for the time being because it would be a pretty rare case to use `is_constant_evaluated` there.

@cor3ntin 
BTW, can I separate the initializer evaluation context bug fix part to make another PR? Another patch I'm working on locally also suffers from the evaluation context bug, and I want to land that part relatively soon.
Also, it might be nice to separate the NFC libc++ test modifications I recently uploaded because it's too much despite being NFC. Or we can turn off this tautology warning against macros. Do you think it's reasonable?


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

https://reviews.llvm.org/D155064