This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Small cleanups to cse.c


Hi Steven,

On Sat, 25 Nov 2006, Steven Bosscher wrote:
> On 11/25/06, Eric Botcazou <ebotcazou@libertysurf.fr> wrote:
> > > Actually, that looks like a mistake.  I should have typed
> > >
> > >   if (!any_condjump_p (insn))
> > >     gcc_unreachable ();
> > >
> > > there, without the ENABLE_CHECKING.
> >
> > Not the correct idiom either. :-)
>
>
> Well, not incorrect, just different. You'll find the form I suggested
> in a lot of places in GCC as well.
>
> > You probably simply want:
> >
> >   gcc_assert (any_condjump_p (insn));
>
> This would also work, but in this form we wouldn't check that insn has
> the valid form if gcc is configured with assert checking disabled.
>
> I prefer to err on the safe side and check this even with assert
> checking disabled ;-)  But I don't feel strongly about it and I can
> go with your suggestion if that's what most other folks prefer.


I'm with Eric on this one and would prefer the more usual:
    gcc_assert (any_condjump_p (insn));

This is precisely the intended use of gcc_assert, and anyone turning
off assertion checking presumably explicitly intends to disable this
form of internal consistency checking.


I did do a quick audit of the gcc/ directory to see if I could find
any instances of "if (foo) gcc_unreachable();"  Although not exhaustive,
the vast majority of uses of gcc_unreachable are in switch statements
for handling invalid cases, or at the very end of long
if-then-else-if-then-else chains.

I did find one instance in builtins.c:expand_builtin_int_roundingfn
(though as I said I wasn't exhaustive) that contained:

  if (!validate_arglist (arglist, REAL_TYPE, VOID_TYPE))
    gcc_unreachable ();

which I think really should be just gcc_assert (or to match the
rest of builtins.c, it should just return NULL_RTX!).  I'm sure
there may be legitimate counter examples, (perhaps involving
functions with side-effects) but I support Eric's view that the
"rule of thumb" is to always use gcc_assert.


Thanks in advance for making that change.  This clean-up looks
good, BTW.  It looks like this was the last remaining use of
USE_C_ALLOCA in the tree, no remaining backends define it, so
we should also remove the documentation from hostconfig.texi.

Roger
--


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]