This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: RFA (tree.c): PATCH for c++/68782 (wrong TREE_CONSTANT flag on C++ CONSTRUCTOR)
- From: Jakub Jelinek <jakub at redhat dot com>
- To: Jason Merrill <jason at redhat dot com>
- Cc: gcc-patches List <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 26 Jan 2016 21:32:13 +0100
- Subject: Re: RFA (tree.c): PATCH for c++/68782 (wrong TREE_CONSTANT flag on C++ CONSTRUCTOR)
- Authentication-results: sourceware.org; auth=none
- References: <56A7D4F4 dot 5020102 at redhat dot com>
- Reply-to: Jakub Jelinek <jakub at redhat dot com>
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