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

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet marked an inline comment as done.
hazohelet added inline comments.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:2440
+            S.ExprEvalContexts.back().Context;
+        if ((D.getDeclSpec().getTypeQualifiers() == DeclSpec::TQ_const ||
+             isNonlocalVariable(ThisDecl)) &&
----------------
cor3ntin wrote:
> Why are we looking at whether static vars are const qualified?
```
void func() { // IsRuntimeEvaluated
	int        A = std::is_constant_evaluated(); // false
	const int  B = std::is_constant_evaluated(); // not tautology
	static int C = std::is_constant_evaluated(); // not tautology
}
```
Variable initializers can inherit the enclosing evaluation scope's `IsRuntimeEvaluated` attribute in most cases, but when the enclosing scope is `IsRuntimeEvaluated` and the variable is const-qualified or non-local, the initializer is not `IsRuntimeEvaluated`.
Manually setting context kind to `PotentiallyEvaluated` is redundant, so I'll remove it.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:17962-18006
-/// Determine whether the given declaration is a global variable or
-/// static data member.
-static bool isNonlocalVariable(const Decl *D) {
-  if (const VarDecl *Var = dyn_cast_or_null<VarDecl>(D))
-    return Var->hasGlobalStorage();
-
-  return false;
----------------
cor3ntin wrote:
> hazohelet wrote:
> > cor3ntin wrote:
> > > Can you explain the changes to this file? they seems to affect test cases unrelated to the goal of this PR
> > When we push/pop evaluation context here, it behaves synchronously with `InitializerScopeRAII`, in ParseDecl.cpp, which makes the evaluation of `Actions.AddInitializerToDecl` happen outside of the initializer expression evaluation context (e.g. initializer scope popped in 2581 and AddInitializerToDecl evaluated in 2592 in ParseDecl.cpp). This is causing a bug BEFORE this patch (https://godbolt.org/z/b18jxKT54 and the removed FIXME in test/SemaCXX/cxx2a-consteval.cpp) because  we were pushing evaluation context if the variable `isNonlocalVariable`. This PR separates the handling of expression evaluation context of initializers from `InitializerScopeRAII` to resolve these bugs.
> > 
> > The arguable changes in diagnostics caused by this are the removals of
> > `warn_uninit_self_reference_in_reference_init` against `constexpr int &n = n;` (You mentioned) and
> > `warn_impcast_integer_precision_constant` against `constexpr signed char a = 2*100;`
> > Both are diagnosed by calling `Sema::DiagRuntimeBehavior`, and `Sema::DiagIfReachable` called by this intentionally avoids emitting diagnostics against initializer of constexpr variables. (https://github.com/llvm/llvm-project/blob/ab73bd3897b58ecde22ec5eea14b6252f2d69b6e/clang/lib/Sema/SemaExpr.cpp#L20712-L20719)
> > They were diagnosed BEFORE this patch because the expression evaluation context was outside of the initializer evaluation context, and it emitted false positives against cases like `constexpr signed char a = true ? 3 : 200;` (https://godbolt.org/z/9z1rer7E3)
> > I think if the evaluated result of constexpr variable initializer does not fit the variable type it should be diagnosed, but the logic should be added to where we handle constexpr variables.
> > 
> Thanks for the detailed explanation.
> Separating InitializerScopeRAII from evaluation scope is something i support.
> Did you try removing the test in `DiagIfReachable`, see how much breaks?
> In any case, there are 2 fixme in that function already, that should cover us.
> It might be useful to better describe these changes in the release notes / commit message
I forgot to write that this PR pushes `ConstantEvaluated` on constexpr var initializers and thus `DiagRuntimeBehavior` returns before calling `DiagIfReachable`.

Locally, I tried making `DiagRuntimeBehavior` emit diagnostics on constexpr var initializers. It causes test failure in 7 test files and the added diagnostics seem redundant.
I don't list all of them here, but especially problematic would be variable template: https://github.com/llvm/llvm-project/blob/e8dc9dcd7df798039ccf23d74343bf688b1fd648/clang/test/CXX/temp/temp.constr/temp.constr.constr/non-function-templates.cpp#L11-L16 . Clang would emit false positive about narrowing conversion every time it gets instantiated.
So, I don't think it's a good idea to let DiagRuntimeBehavior emit diagnostics on constexpr var initializers.
I'll better describe the change here, thanks.


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

https://reviews.llvm.org/D155064