Re: [Tuples] Cleaned up accessor patch.

[Diego Novillo <dnovillo at google dot com>]

On 6/22/07 1:10 PM, Christopher Matthews wrote:

>  I also switched the gs_seqs to use accessors instead of macros.  One
> thing I couldn't decided on was whether gs_seq_first, and gs_seq_last
> should return the gimple element, or a pointer to it. It seemed more
> natural in the gimple_itterator to return the pointer, so that is the
> way I left it.  I'm not really happy with it though.

I don't really see why we need a pointer here.  Aldy, do you remember
why we had a 'gimple *' instead of 'gimple' in gimple_stmt_iterator?  If
there wasn't a good reason, let's drop the pointer.


>     * function.c (gimplify-oaraneters): Changed to gs_seq accessors.

There should be a hard <tab> before each CL line.

>  
> -  i.ptr = &GS_SEQ_FIRST (seq);
> +  i.ptr = gs_seq_first(seq);

Space before '('.

> -   FINALLY, does this try have a finally?  */
> +   CATCH_P, does this try catch?

CATCH_P is true if this is a try/catch.

> +   FINALLY_P, does this try have a finally?  */

FINALLY_P is true if this is a try/finally

> +   BODY is sequence of statements inside the for loop.
> +   CLAUSES, are any of the OMP loop consturct's clauses: private, firstprivate, 

s/consturct/construct/

> +   lastprivate, reductions, ordered, schedule, and nowait.
> +   PRE_BODY is the sequence of statements that are loop invarient.

s/invarient/invariant/

> +   BODY is sequence of statements which are executed in parallel.
> +   CLAUSES, are the OMP parallel consturct's clauses.

s/consturct/construct

>  
> -/* Construct a GS_OMP_RETURN statement for BODY.  */
> +/* Construct a GS_OMP_RETURN statement.
> +   FIXME: BODY.  */
>  
Hmm?  OMP_RETURN has no body.  This routine should at most get one
argument 'wait_p' to toggle OMP_RETURN_NOWAIT.


 > -  if (GS_SEQ_FIRST (seq) == NULL)
> +  if (*gs_seq_first (seq) == NULL)

No.  You don't want to dereference this.  More reason not to put a
'gimple *' as the field.  It's confusing and I don't think it's really
needed.


> +static inline gimple*

'gimple *', but I actually think we want 'gimple' here.

> +static inline bool
> +gs_seq_empty_p(gs_seq s) {
> +  return !(s->first);

return s->first == NULL is a tad clearer.