On Thu, Jun 10, 2021 at 10:09:26AM -0400, Jason Merrill wrote:
> > --- gcc/cp/parser.c.jj 2021-06-09 21:54:39.482193853 +0200
> > +++ gcc/cp/parser.c 2021-06-10 10:09:23.753052980 +0200
> > @@ -10902,6 +10902,11 @@ cp_parser_lambda_expression (cp_parser*
> > bool discarded = in_discarded_stmt;
> > in_discarded_stmt = 0;
> > + /* Similarly the body of a lambda in immediate function context is not
> > + in immediate function context. */
> > + bool immediate_fn_ctx_p = in_immediate_fn_ctx_p;
>
> It's hard to distinguish between these two names when reading the code;
> let's give the local variable a name including "saved".
Will do.
> > + if (cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE))
> > + error ("%<if consteval%> requires compound statement");
> > +
> > + in_immediate_fn_ctx_p |= ce > 0;
> > + cp_parser_implicitly_scoped_statement (parser, NULL, guard_tinfo);
>
> Maybe use cp_parser_compound_statement directly instead of this and checking
> CPP_OPEN_BRACE above? Either way is fine.
I thought doing it this way will provide better diagnostics for what I think
can be a common bug - people so used to normal if not requiring compound
statements forgetting those {}s from time to time.
> > + if (TREE_CODE (t) == IF_STMT && IF_STMT_CONSTEVAL_P (t))
> > + {
> > + /* Evaluate the condition as if it was
> > + if (__builtin_is_constant_evaluated ()). */
> > + if (ctx->manifestly_const_eval)
> > + val = boolean_true_node;
> > + else
> > + {
> > + *non_constant_p = true;
> > + return t;
> > + }
>
> Why set *non_constant_p? Shouldn't this just be
>
> val = boolean_false_node;
>
> so we constant-evaluate the else clause when we are trying to
> constant-evaluate in a non-manifestly-constant-evaluated context?
It matches what we do for CP_BUILT_IN_IS_CONSTANT_EVALUATED calls:
/* For __builtin_is_constant_evaluated, defer it if not
ctx->manifestly_const_eval, otherwise fold it to true. */
if (fndecl_built_in_p (fun, CP_BUILT_IN_IS_CONSTANT_EVALUATED,
BUILT_IN_FRONTEND))
{
if (!ctx->manifestly_const_eval)
{
*non_constant_p = true;
return t;
}
return boolean_true_node;
}
I believe we sometimes try to constexpr evaluate something without
having manifestly_const_eval = true even on expressions that
are in manifestly constant evaluated contexts and am worried if
we just folded it to boolean_false_node we could get a constant expression
and replace the expression with that, even before we actually try to
constant evaluate it with manifestly_const_eval = true.
If I do (for the CP_BUILT_IN_IS_CONSTANT_EVALUATED):
--- gcc/cp/constexpr.c.jj 2021-06-10 15:27:31.123353594 +0200
+++ gcc/cp/constexpr.c 2021-06-10 16:26:38.368168281 +0200
@@ -1320,10 +1320,7 @@ cxx_eval_builtin_function_call (const co
BUILT_IN_FRONTEND))
{
if (!ctx->manifestly_const_eval)
- {
- *non_constant_p = true;
- return t;
- }
+ return boolean_false_node;
return boolean_true_node;
}
then
FAIL: g++.dg/cpp2a/is-constant-evaluated1.C -std=c++14 execution test
FAIL: g++.dg/cpp2a/is-constant-evaluated1.C -std=c++17 execution test
FAIL: g++.dg/cpp2a/is-constant-evaluated1.C -std=c++2a execution test
FAIL: g++.dg/cpp2a/is-constant-evaluated1.C -std=c++17 -fconcepts execution
test
FAIL: g++.dg/cpp2a/is-constant-evaluated2.C -std=c++14 execution test
FAIL: g++.dg/cpp2a/is-constant-evaluated2.C -std=c++17 execution test
FAIL: g++.dg/cpp2a/is-constant-evaluated2.C -std=c++2a execution test
FAIL: g++.dg/cpp2a/is-constant-evaluated2.C -std=c++17 -fconcepts execution
test
FAIL: g++.dg/cpp2a/is-constant-evaluated9.C -std=gnu++2a (test for excess
errors)
at least regresses.
> > + if (TREE_CODE (t) != IF_STMT || !IF_STMT_CONSTEVAL_P (t))
> > + {
> > + if (integer_zerop (tmp))
> > + return RECUR (TREE_OPERAND (t, 2), want_rval);
> > + else if (TREE_CODE (tmp) == INTEGER_CST)
> > + return RECUR (TREE_OPERAND (t, 1), want_rval);
> > + }
>
> Don't we still want to shortcut consideration of one of the arms for if
> consteval?
potential_constant_expression_p{,_1} doesn't get passed whether it is
manifestly_const_eval or not.
> > --- gcc/cp/cp-gimplify.c.jj 2021-06-09 21:54:39.473193977 +0200
> > +++ gcc/cp/cp-gimplify.c 2021-06-10 09:49:35.898557178 +0200
> > @@ -161,7 +161,9 @@ genericize_if_stmt (tree *stmt_p)
> > if (!else_)
> > else_ = build_empty_stmt (locus);
> > - if (integer_nonzerop (cond) && !TREE_SIDE_EFFECTS (else_))
> > + if (IF_STMT_CONSTEVAL_P (stmt))
> > + stmt = else_;
>
> This seems redundant, since you're using boolean_false_node for the
> condition.
It is only when !TREE_SIDE_EFFECTS (else_).
I think that is about having labels in the then_/else_ blocks and
gotos jumping into those from outside.
But IF_STMT_CONSTEVAL_P guarantees that is not the case (and we verify
earlier), so we can do that (and in fact, for IF_STMT_CONSTEVAL_P have
to, because then_ could contain consteval calls not constant expression
evaluated).
I guess we could do that for IF_STMT_CONSTEXPR_P too, that also
doesn't allow gotos/switch into the branches.
> > --- gcc/cp/call.c.jj 2021-06-09 21:54:39.436194489 +0200
> > +++ gcc/cp/call.c 2021-06-10 09:49:35.949556470 +0200
> > @@ -8840,6 +8840,7 @@ immediate_invocation_p (tree fn, int nar
> > || !DECL_IMMEDIATE_FUNCTION_P (current_function_decl))
> > && (current_binding_level->kind != sk_function_parms
> > || !current_binding_level->immediate_fn_ctx_p)
> > + && !in_immediate_fn_ctx_p
>
> Now that we have this flag, shouldn't we set it in actual immediate function
> context? Or rename the flag to match when it's actually set.
I guess I can try that. Though, I'm not sure if we could also get rid of
the current_binding_level->immediate_fn_ctx_p for sk_function_parms case.
Jakub
