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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
