Re: [C++ PATCH] Fix __builtin_{is_constant_evaluated,constant_p} handling in static_assert (PR c++/86524, PR c++/88446)

[Jason Merrill <jason@redhat.com>]

On 12/20/18 5:27 PM, Jakub Jelinek wrote:
> On Thu, Dec 20, 2018 at 04:47:29PM -0500, Jason Merrill wrote:
>>> So are you ok with what is in the patch below, i.e.
>>>          {
>>>            bool non_cst_p = false, ovf_p = false;
>>>            tree a = cxx_eval_constant_expression (&new_ctx, args[i], false,
>>>                                                   &non_cst_p, &ovf_p);
>>>            if ((!non_cst_p && !ovf_p) || !ctx->manifestly_const_eval)
>>>              args[i] = a;
>>>          }
>>> , or perhaps without the || !ctx->manifestly_const_eval?
>>
>> I don't see how that makes a difference from what was there before; if the
>> argument to cxx_eval_constant_expression is non-constant, it returns the
>> argument unchanged.
> 
> If that is guaranteed, then it is ok to keep it as is I guess.
> Will change it then.
> 
>>> So, if the
>>> argument is a constant expression, fold to that, if it is not, just do
>>> cp_fully_fold on it if it is __builtin_constant_p, otherwise nothing?
>>
>> Hmm, cp_fully_fold probably also needs to add a manifestly_const_eval
>> parameter to pass along to maybe_constant_value.
> 
> But if we need cp_fully_fold, doesn't that mean that the earlier
> cxx_eval_constant_expression failed and thus the argument is not a constant
> expression?  Should __builtin_is_constant_evaluated () evaluate to true
> even if the argument is not a constant expression?

Ah, no, good point.

> Say if there is
> int v;
> constexpr int foo (void)
> {
>    return __builtin_constant_p (v * (__builtin_is_constant_evaluated () ? 1 : 0));
> }
> Because v is not a constant expression,
> v * (__builtin_is_constant_evaluated () ? 1 : 0) shouldn't be either.
> 
> cp_fully_fold does:
>    /* FIXME cp_fold ought to be a superset of maybe_constant_value so we don't
>       have to call both.  */
>    if (cxx_dialect >= cxx11)
>      {
>        x = maybe_constant_value (x);
>        /* Sometimes we are given a CONSTRUCTOR but the call above wraps it into
>           a TARGET_EXPR; undo that here.  */
>        if (TREE_CODE (x) == TARGET_EXPR)
>          x = TARGET_EXPR_INITIAL (x);
>        else if (TREE_CODE (x) == VIEW_CONVERT_EXPR
>                 && TREE_CODE (TREE_OPERAND (x, 0)) == CONSTRUCTOR
>                 && TREE_TYPE (TREE_OPERAND (x, 0)) == TREE_TYPE (x))
>          x = TREE_OPERAND (x, 0);
>      }
>    return cp_fold_rvalue (x);
> Is there a reason to call that maybe_constant_value at all when we've called
> cxx_eval_constant_expression first?  Wouldn't cp_fold_rvalue (or
> c_fully_fold with false as last argument) be sufficient there?

I think that would be better, yes.

Jason