hazohelet marked 9 inline comments as done.
hazohelet added a comment.
Thank you for the review!
================
Comment at: clang/include/clang/Sema/Sema.h:1277
+ /// as PotentiallyEvaluated.
+ RuntimeEvaluated
};
----------------
cor3ntin wrote:
> I think I'd prefer a boolean for that, `ExpressionEvaluationContext` somewhat
> matches standard definitions.
> I think we might mostly be able to reuse PotentiallyEvaluatedIfUsed |
> PotentiallyEvaluated.
>
> RuntimeEvaluated is anything that does not have a parent context that is
> constant evaluated/immediate/unevaluated. Maybe you could try to make a
> function for that?
I made it a boolean instead of a new expression evaluation context.
> RuntimeEvaluated is anything that does not have a parent context that is
> constant evaluated/immediate/unevaluated. Maybe you could try to make a
> function for that?
I dont't think it is correct. The definitions body of constexpr-qualified
function and non-qualified function both push `PotentiallyEvaluated`, so we
need some way to distinguish them, and this time it is a boolean.
================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:3214-3221
EnterExpressionEvaluationContext Context(
Actions,
isa_and_present<FieldDecl>(D)
? Sema::ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed
: Sema::ExpressionEvaluationContext::PotentiallyEvaluated,
D);
+ Actions.ExprEvalContexts.back().InConstantEvaluated =
----------------
cor3ntin wrote:
> Maybe we should instead say that a constexpr FieldDecl is ConstantEvaluated ?
> `InConstantEvaluated` in general seems redundant
I removed `InConstantEvaluated`
================
Comment at: clang/lib/Parse/ParseStmt.cpp:1513-1516
+ EnterExpressionEvaluationContext Consteval(
+ Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated,
+ /*LambdaContextDecl=*/nullptr,
+ Sema::ExpressionEvaluationContextRecord::EK_Other, IsConstexpr);
----------------
cor3ntin wrote:
> This seems wrong, the condition of a if statement is not constant evaluated.
In this case `/*ShouldEnter=*/IsConstexpr`, so the constant evaluation context
is pushed only when it is a condition of constexpr if. The condition of
constexpr if shall be a constant expression as per
https://eel.is/c++draft/stmt.if#2
================
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:
> 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.
================
Comment at: clang/test/SemaCXX/constant-expression-cxx11.cpp:1964
void f() {
- constexpr int &n = n; // expected-error {{constant expression}}
expected-note {{use of reference outside its lifetime}} expected-warning {{not
yet bound to a value}}
constexpr int m = m; // expected-error {{constant expression}}
expected-note {{read of object outside its lifetime}}
----------------
cor3ntin wrote:
> This change does not seem correct
This is because it is diagnosed by `Sema::DiagRuntimeBehavior`
In this case constexpr evaluator is emitting an error, so I don't think the
loss of this warning is problematic.
Please see my comment about the changes in ParseDeclCXX.cpp for details
================
Comment at:
libcxx/test/std/utilities/meta/meta.const.eval/is_constant_evaluated.verify.cpp:27
static_assert(!std::is_constant_evaluated(), "");
- // expected-warning@-1 0-1 {{'std::is_constant_evaluated' will always
evaluate to 'true' in a manifestly constant-evaluated expression}}
+ // expected-warning@-1 0-1 {{'std::is_constant_evaluated' will always
evaluate to true in this context}}
#endif
----------------
philnik wrote:
> Mordante wrote:
> > Since libc++ support the latest ToT Clang and the last two official
> > releases this wont work. The `expected-warning` needs to be a
> > `expected-warning-re` that works for both the new and old diagnostic
> You can also just shorten it to `'std::is_constant_evaluated' will always
> evaluate to`. Seems good enough to me.
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
