On 6/10/21 10:44 AM, Jakub Jelinek wrote:
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.
What the patch currently diagnoses is:
consteval-if2.C:13:6: error: ‘if consteval’ requires compound statement
13 | if consteval if (true) {} // { dg-error "'if consteval' requires
compound statement" }
| ^~~~~~~~~
while with cp_parser_compound_statement directly it diagnoses:
consteval-if2.C:13:16: error: expected ‘{’ before ‘if’
13 | if consteval if (true) {} // { dg-error "'if consteval' requires
compound statement" }
| ^~
What do you prefer?
The second is clearer about the fix, the first is clearer about the
problem. Maybe add a fixit to the first error?
Dunno if the fine detail that in the grammar only one of the statements
is compound-statement and then there is a requirement that the other
statement has to be a compound-statement shouldn't affect how it is
reported.
The difference was to prevent "else ;" from binding to an enclosing if
rather than the if consteval.
+ 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.
OK, just add a comment then.
+ 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.
OK, just add a comment then.
--- 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
