Re: [PATCH 5/6] Port various places from union access to subclass access.

[Jeff Law <law@redhat.com>]

On 10/31/13 10:26, David Malcolm wrote:
>
> diff --git a/gcc/gimple-streamer-in.c b/gcc/gimple-streamer-in.c
> index 4f31b83..2555dbe 100644
> --- a/gcc/gimple-streamer-in.c
> +++ b/gcc/gimple-streamer-in.c
> @@ -129,13 +129,14 @@ input_gimple_stmt (struct lto_input_block *ib, struct data_in *data_in,
>       case GIMPLE_ASM:
>         {
>   	/* FIXME lto.  Move most of this into a new gimple_asm_set_string().  */
> +	gimple_statement_asm *asm_stmt = as_a <gimple_statement_asm> (stmt);
>   	tree str;
> -	stmt->gimple_asm.ni = streamer_read_uhwi (ib);
> -	stmt->gimple_asm.no = streamer_read_uhwi (ib);
> -	stmt->gimple_asm.nc = streamer_read_uhwi (ib);
> -	stmt->gimple_asm.nl = streamer_read_uhwi (ib);
> +	asm_stmt->ni = streamer_read_uhwi (ib);
> +	asm_stmt->no = streamer_read_uhwi (ib);
> +	asm_stmt->nc = streamer_read_uhwi (ib);
> +	asm_stmt->nl = streamer_read_uhwi (ib);
>   	str = streamer_read_string_cst (data_in, ib);
> -	stmt->gimple_asm.string = TREE_STRING_POINTER (str);
> +	asm_stmt->string = TREE_STRING_POINTER (str);
The in/out streaming seems like another natural fit for virtual 
functions in the long term.


>         }
>         /* Fallthru  */
>
> diff --git a/gcc/gimple.c b/gcc/gimple.c
> index 9b1337a..e9ef8e0 100644
> --- a/gcc/gimple.c
> +++ b/gcc/gimple.c
> @@ -641,21 +641,22 @@ static inline gimple
>   gimple_build_asm_1 (const char *string, unsigned ninputs, unsigned noutputs,
>                       unsigned nclobbers, unsigned nlabels)
>   {
> -  gimple p;
> +  gimple_statement_asm *p;
>     int size = strlen (string);
>
>     /* ASMs with labels cannot have outputs.  This should have been
>        enforced by the front end.  */
>     gcc_assert (nlabels == 0 || noutputs == 0);
>
> -  p = gimple_build_with_ops (GIMPLE_ASM, ERROR_MARK,
> -			     ninputs + noutputs + nclobbers + nlabels);
> +  p = as_a <gimple_statement_asm> (
> +        gimple_build_with_ops (GIMPLE_ASM, ERROR_MARK,
> +			       ninputs + noutputs + nclobbers + nlabels));
>
> -  p->gimple_asm.ni = ninputs;
> -  p->gimple_asm.no = noutputs;
> -  p->gimple_asm.nc = nclobbers;
> -  p->gimple_asm.nl = nlabels;
> -  p->gimple_asm.string = ggc_alloc_string (string, size);
> +  p->ni = ninputs;
> +  p->no = noutputs;
> +  p->nc = nclobbers;
> +  p->nl = nlabels;
> +  p->string = ggc_alloc_string (string, size);
As noted in a prior message, having build methods would eliminate this 
downcasting.  Not necessary for this patch, just wanted to point it out 
to anyone reading.  I won't point it out everywhere ;-)


So given the prior disussions around as_a, I'm not going to object to 
these as_a instances.  I see them as warts/markers that we have further 
work to do in terms of fleshing out the class and possibly refactoring 
code.

Conditionally OK.  Conditional on the other related patches going in and 
keeping it updated with Andrew's churn.  If/when the set goes in, post 
the final version you actually checkin -- no re-review is needed for 
this hunk so long as any changes are the obvious fixing of fallout from 
Andrew's work.

Jeff