Re: [PATCH] Make CONSTRUCTOR use VEC

["Giovanni Bajo" <rasky at develer dot com>]

Nathan Sidwell <nathan@codesourcery.com> wrote:

> Index: cp/decl.c
>       /* An array of character type can be initialized from a
> ! brace-enclosed string constant.
> !
> ! FIXME: this code is duplicated from reshape_init. Probably
> ! we should just call reshape_init here?  */
>
> I think the fixme is correct, but I'm happy for that to be in a followup
> patch.

OK.

> Index: cp/decl2.c
>     if (TREE_CODE (init) == TREE_LIST)
>       {
> +       gcc_unreachable ();
>         if (TREE_CHAIN (init) == NULL_TREE)
>   init = TREE_VALUE (init);
>         else
> ! init = digest_init (TREE_TYPE (value), init);
>       }
>
> why not just 'assert (TREE_CODE (init) != TREE_LIST)'?

I forgot to clean it up: I'll just remove the whole block


> Index: cp/pt.c
> *************** tsubst_copy (tree t, tree args, tsubst_f
> + gcc_assert (0);  /* FIXME: is this dead code?  */
>
> I think this is unreachable.  Tracing the ways we can reach tsubst_copy
are
> 1) for a using_decl's scope and name -- neither will be a ctor
> 2) for a typename type's full name - a template id-expr, which cannot
> contain a constructor
> 3) the name of a qualified id.  cannot be a constructor
> 4) the member name in a component_ref.  canot be a constructor
> 5) a parm_decl or var_decl
> 6) the type of a va_arg_expr
> 7) a const_decl
> 8) other cases not explicitly handled in tsubst_copy_and_build
> 9) the decl part of a tsubst_initializer_list
> 10) recursively
>
> AFAICT none of these will be a constructor_node, neither can they contain
a
> constructor node so we won't reach that code recursively.
>
> You can change that whole block of code to a gcc_unreachable () with a
> comment that tsubst_copy_and_build should have been used if it ever is
> reached.

Another piece of code I forgot to clean up, but thanks for your exhaustive
analysys.
BTW, I don't think removing tsubst_copy at this point is a big effort. I'll
see if I can find a spare week.

> Index: cp/tree.c
>         if (TREE_CODE (t) == CONSTRUCTOR
> !   && EMPTY_CONSTRUCTOR_P (t))
>
> EMPTY_CONSTRUCTOR_P also checks that t is a constructor.  Perhaps it
should
> only be given CONSTRUCTORs?  I see you haven't changed that behaviour, so
> maybe it should be dealt with in a subsequent patch, if at all.

There is much confusion here, yes, and redundant checks within macros. I
tried cleaning up some of them, but it was not trivial and the patch was
already too big. The first thing I would like to clean up in a follow up
patch is moving compound literals to their own node (COMPOUND_LITERAL_EXPR
for instance) instead of reusing CONSTRUCTORs with a lang flag (which btw,
it's called TREE_HAS_CONSTRUCTOR, to increase the confusion).

> Index: cp/typeck2.c
> !       /* Remove the element from the vector (O(1) operation). By
> !          relying on the field of each element (and thus being able
> ! to change the order of the elements), we can avoid quadratic
> ! behavior.  */
>
> you mentioned this.  I think inserting 'FIXME' just here would be good.

Actually, the comment is outdated. We cannot rely on the field being set
because middle-end expects CONSTRUCTOR's elements to be sorted. I've updated
the comment:

       /* FIXME: Ordered removal is O(1) so the whole function is
   worst-case quadratic. This could be fixed using an aside
   bitmap to record which elements must be removed and remove
   them all at the same time. Or by merging
   split_non_constant_init into process_init_constructor_array,
   that is separating constants from non-constants while building
   the vector.  */

> !   /* Come here only for records and arrays (and unions with
> constructors).  */
> Again, not something you've changed, but unions don't
> have constructors.  I wonder what's going on.

Maybe the comment just means CONSTRUCTORs, as in constructor nodes (not
"constructor" in the C++ sense). I've updated the comment:

  /* Come here only for aggregates: records, arrays, unions, complex numbers
     and vectors.  */


> The changes to coverage.c are ok too, if you promise to deal with the
> fixme's you've put in promptly :)
> !   /* FIXME: use build_constructor directly.  */

OK. Is 'make bootstrap && make -k check' enough to test coverage.c? Or will
I need something else?

> Once again, thanks for tackling this issue!

No prob! BTW, I assume you also consider the vec.h changes ok?

I'll post an updated version of the patch with these C++ fixes in the
weekend when I ping it for review.
Thanks for the review!
-- 
Giovanni Bajo