This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Tuples] Cleaned up accessor patch.
- From: Diego Novillo <dnovillo at google dot com>
- To: Christopher Matthews <chrismatthews at google dot com>
- Cc: Aldy Hernandez <aldyh at redhat dot com>, gcc-patches at gcc dot gnu dot org
- Date: Fri, 22 Jun 2007 15:04:48 -0400
- Subject: Re: [Tuples] Cleaned up accessor patch.
- References: <5794c4c80706221010r38754e7cvf037467650a9d45e@mail.gmail.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.