RFA (tree.c): PATCH for c++/68782 (wrong TREE_CONSTANT flag on C++ CONSTRUCTOR)

[Jakub Jelinek]

On Tue, Jan 26, 2016 at 03:20:04PM -0500, Jason Merrill wrote:
> Tested x86_64-pc-linux-gnu. Are the tree.c changes OK for trunk?

The tree.c changes are ok.  But I have nits and one bigger issue in
constexpr.c:

> --- a/gcc/cp/constexpr.c
> +++ b/gcc/cp/constexpr.c
> @@ -2214,6 +2214,9 @@ cxx_eval_bare_aggregate (const constexpr_ctx *ctx, tree t,
>    vec<constructor_elt, va_gc> **p = &CONSTRUCTOR_ELTS (ctx->ctor);
>    vec_alloc (*p, vec_safe_length (v));
>  
> +  bool constant_p = true;
> +  bool side_effects_p = false;
> +
>    unsigned i; tree index, value;

I think vars of different types shouldn't be declared on the same line.
And perhaps the empty line in between two sets of declarations isn't needed.

>    FOR_EACH_CONSTRUCTOR_ELT (v, i, index, value)
>      {
> @@ -2826,6 +2836,8 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
>      }
>    type = TREE_TYPE (object);
>    bool no_zero_init = true;
> +
> +  vec<tree,va_gc> *ctors = make_tree_vector();

Missing space before (.

>    while (!refs->is_empty())
>      {
>        if (*valp == NULL_TREE)
> @@ -2837,6 +2849,8 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
>  	 subobjects will also be zero-initialized.  */
>        no_zero_init = CONSTRUCTOR_NO_IMPLICIT_ZERO (*valp);
>  
> +      vec_safe_push (ctors, *valp);
> +
>        enum tree_code code = TREE_CODE (type);
>        type = refs->pop();
>        tree index = refs->pop();
> @@ -2889,14 +2903,35 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
>        /* The hash table might have moved since the get earlier.  */
>        valp = ctx->values->get (object);
>        if (TREE_CODE (init) == CONSTRUCTOR)
> -	/* An outer ctx->ctor might be pointing to *valp, so just replace
> -	   its contents.  */
> -	CONSTRUCTOR_ELTS (*valp) = CONSTRUCTOR_ELTS (init);
> +	{
> +	  /* An outer ctx->ctor might be pointing to *valp, so replace
> +	     its contents.  */
> +	  CONSTRUCTOR_ELTS (*valp) = CONSTRUCTOR_ELTS (init);
> +	  TREE_CONSTANT (*valp) = TREE_CONSTANT (init);
> +	  TREE_SIDE_EFFECTS (*valp) = TREE_SIDE_EFFECTS (init);
> +	}
>        else
>  	*valp = init;
>      }
>    else
> -    *valp = init;
> +    {
> +      *valp = init;
> +
> +      /* Update TREE_CONSTANT and TREE_SIDE_EFFECTS on enclosing
> +	 CONSTRUCTORs.  */
> +      unsigned i; tree elt;
> +      bool c = TREE_CONSTANT (init);
> +      bool s = TREE_SIDE_EFFECTS (init);
> +      if (!c || s)
> +	FOR_EACH_VEC_SAFE_ELT (ctors, i, elt)
> +	  {
> +	    if (!c)
> +	      TREE_CONSTANT (elt) = false;
> +	    if (s)
> +	      TREE_SIDE_EFFECTS (elt) = true;
> +	  }
> +    }
> +  release_tree_vector (ctors);
>  
>    if (*non_constant_p)
>      return t;
> @@ -3579,9 +3614,16 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
>  
>      case CONSTRUCTOR:
>        if (TREE_CONSTANT (t))
> -	/* Don't re-process a constant CONSTRUCTOR, but do fold it to
> -	   VECTOR_CST if applicable.  */
> -	return fold (t);
> +	{
> +	  /* Don't re-process a constant CONSTRUCTOR, but do fold it to
> +	     VECTOR_CST if applicable.  */
> +	  if (CHECKING_P)
> +	    verify_constructor_flags (t);
> +	  else
> +	    recompute_constructor_flags (t);

But I don't understand this.  Either the flags are supposed to be already
correct here, then I'd expect to see
  if (CHECKING_P)
    verify_constructor_flags (t);
only, or they are not guaranteed to be correct, and then I'd expect
unconditional
  recompute_constructor_flags (t).

> +	  if (TREE_CONSTANT (t))
> +	    return fold (t);
> +	}
>        r = cxx_eval_bare_aggregate (ctx, t, lval,
>  				   non_constant_p, overflow_p);
>        break;

	Jakub