Re: [patch] tree-phinode.c: Clean up remove_phi_node.

[Jeffrey A Law <law at redhat dot com>]

On Thu, 2005-03-03 at 12:25 -0500, Kazu Hirata wrote:
> Hi Jeff,
> 
> > This is fine.
> 
> Thanks.
> 
> > I will note the behavior of the new code is not exactly the same as
> > the old code.
> > 
> > Specifically if the old code would do nothing if the passed in PHI
> > was not in the chain of PHI nodes attached to BB.  I'm pretty sure
> > the new code will segfault.
> > 
> > You might consider a follow-up patch which gives a more graceful
> > failure mode via gcc_assert rather than segfaulting :-)
> 
> Well, I was aware of this segfault, and I was thinking that maybe I
> can live with a segfault. :-) I heasitate a little bit to change the
> "for" loop to
> 
>       for (loc = &(bb_ann (bb)->phi_nodes);
> 	   *loc != NULL_TREE && *loc != phi;
> 	   loc = &PHI_CHAIN (*loc))
> 	;
>       gcc_assert (*loc == NULL_TREE);
> 
> because we have two "if"s in the loop condition.  It's not that this
> function is very performance critical, but....  I'll leave the
> decision up to you.  Would you like the "for" loop above?
First, let's lose the "BB" argument to remove_phi_node.  BB should
always be computable from bb_for_stmt (PHI).  This removes the ability
for a caller to goof things up by passing in the wrong BB.

We then make sure that our checkers verify that each PHI node is on
the right chain (we might already do this, I haven't checked).

I would still put in the *loc != NULL check as there would still be
ways to lose and get a segfault.  But those ought to be exceedingly
rare.  You could even put that check inside an ENABLE_CHECKING
ifdef.

Jeff