Re: [PATCH v2] c++: Move consteval folding to cp_fold_r

[Jason Merrill <jason@redhat.com>]

On 9/7/23 11:23, Marek Polacek wrote:
> On Tue, Sep 05, 2023 at 04:36:34PM -0400, Jason Merrill wrote:
>> On 9/5/23 15:59, Marek Polacek wrote:
>>> On Tue, Sep 05, 2023 at 10:52:04AM -0400, Jason Merrill wrote:
>>>> On 9/1/23 13:23, Marek Polacek wrote:
>>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>>>>>
>>>>> -- >8 --
>>>>>
>>>>> In the review of P2564:
>>>>> <https://gcc.gnu.org/pipermail/gcc-patches/2023-August/628747.html>
>>>>> it turned out that in order to correctly handle an example in the paper,
>>>>> we should stop doing immediate evaluation in build_over_call and
>>>>> bot_replace, and instead do it in cp_fold_r.  This patch does that.
>>>>>
>>>>> Another benefit is that this is a pretty significant simplification, at
>>>>> least in my opinion.  Also, this fixes the c++/110997 ICE (but the test
>>>>> doesn't compile yet).
>>>>>
>>>>> The main drawback seems to be that cp_fold_r doesn't process as much
>>>>> code as we did before: uninstantiated templates
>>>>
>>>> That's acceptable, it's an optional diagnostic.
>>>>
>>>>> and things like "false ? foo () : 1".
>>>>
>>>> This is a problem.  Maybe we want cp_fold_r to recurse into the arms of a
>>>> COND_EXPR before folding them away?  Maybe only if we know we've seen an
>>>> immediate function?
>>>
>>> Unfortunately we had already thrown the dead branch away when we got to
>>> cp_fold_r.  I wonder if we have to adjust cxx_eval_conditional_expression
>>> to call cp_fold_r on the dead branch too,
>>
>> Hmm, I guess so.
>>
>>> perhaps with a new ff_ flag to skip the whole second switch in cp_fold_r?
>>
>> Or factor out the immediate function handling to a separate walk function
>> that cp_fold_r also calls?
> 
> I did that.
>   
>>> But then it's possible that the in_immediate_context checks have to stay.
>>
>> We can just not do the walk in immediate (or mce_true) context, like we
>> currently avoid calling cp_fold_function.
> 
> Right.  Unfortunately I have to check even when mce_true, consider
> 
>    consteval int bar (int i) { if (i != 1) throw 1; return 0; }
>    constexpr int a = 0 ? bar(3) : 3;

I disagree; the call is in a manifestly constant-evaluated expression, 
and so is now considered an immediate function context, and we should 
accept that example.

>> For mce_unknown I guess we'd want
>> to set *non_constant_p instead of giving an error.
> 
> I did not do this because I haven't found a case where it would make
> a difference.

I think it will given the above comment.

> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> index 0ca4370deab..397d5c7ec3f 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -2311,6 +2311,29 @@ cxx_dynamic_cast_fn_p (tree fndecl)
>   	  && CP_DECL_CONTEXT (fndecl) == abi_node);
>   }
>   
> +/* Return true if we are in the body of a consteval function. > +   This is in addition to in_immediate_context because that
> +   uses current_function_decl which may not be available.  CTX is
> +   the current constexpr context.  */
> +
> +static bool
> +in_immediate_context (const constexpr_ctx *ctx)
> +{
> +  if (in_immediate_context ())
> +    return true;

Can't we check for mce_true here instead of looking at the call chain?

> +/* A wrapper around cp_fold_immediate_r.  */
> +
> +void
> +cp_fold_immediate (tree *tp)
> +{

Maybe return early if consteval isn't supported in the active standard?

Jason