Warning elimination patches in cp/call.c

Warning elimination patches in cp/call.c

[Peter Schmid]

While compiling cp/call.c I get many warnings like:
call.c:3344: warning: enumeration value `NOT_BUILT_IN' not handled in
switch.
The following patch silences these warnings.

Peter  Schmid

Mon Mar  9 21:10:13 1998  Peter Schmid  <schmid@ltoi.iap.physik.tu-darmstadt.de>

	* call.c: (build_over_call) Add default case in enumeration switch

*** gcc/cp/call.c~	Mon Mar  9 11:31:33 1998
--- gcc/cp/call.c	Mon Mar  9 21:05:35 1998
*************** build_over_call (fn, convs, args, flags)
*** 3341,3346 ****
--- 3341,3350 ----
  	if (converted_args == 0)
  	  return integer_zero_node;
  	return build_unary_op (ABS_EXPR, TREE_VALUE (converted_args), 0);
+       
+       default:
+         break;
+               
        }
  
    fn = build_call (fn, TREE_TYPE (TREE_TYPE (TREE_TYPE (fn))), converted_args);

Re: Warning elimination patches in cp/call.c

[Joe Buck]

> While compiling cp/call.c I get many warnings like:
> call.c:3344: warning: enumeration value `NOT_BUILT_IN' not handled in
> switch.
> The following patch silences these warnings.

(add default: break; )

Yes, but maybe that is not the right thing.  It may be that the compiler
expects that other enum values are impossible; if so the default clause
should do an abort, not a break.  Or their might be a mixture of two
cases: enum values for which there should be no action, and values that
are errors.  In that case we need to handle the two cases separately.

Re: Warning elimination patches in cp/call.c

[Tim Hollebeek]

Peter Schmid writes ...
> 
> +
> +       default:
> +         break;
> +               

Does this construct bother anyone else other than me?  In some cases,
it is certainly the correct behavior (when a small number of cases are
handled explicitly, and the rest should be ignored) but if added too
often, it makes the warning it fixes completely useless, since that
particular switch() will no longer _ever_ warn about unhandled cases.
I'd rather see patches that add "case <unhandled enum> : break" after
determining that the value in question really shouldn't be handled.
"default: break" should only be added when the number of "case x :
break" statements gets large.

---------------------------------------------------------------------------
Tim Hollebeek                           | "Everything above is a true
email: tim@wfn-shop.princeton.edu       |  statement, for sufficiently
URL: http://wfn-shop.princeton.edu/~tim |  false values of true."

Re: Warning elimination patches in cp/call.c

[Jeffrey A Law]

  In message < 199803092308.SAA28344@franck.Princeton.EDU >you write:
  > Does this construct bother anyone else other than me?  In some cases,
  > it is certainly the correct behavior (when a small number of cases are
  > handled explicitly, and the rest should be ignored) but if added too
  > often, it makes the warning it fixes completely useless, since that
  > particular switch() will no longer _ever_ warn about unhandled cases.
  > I'd rather see patches that add "case <unhandled enum> : break" after
  > determining that the value in question really shouldn't be handled.
  > "default: break" should only be added when the number of "case x :
  > break" statements gets large.
This is an enormous amount of work, not to mention flat out impossible
since we allow language front ends to make their own tree nodes.  It's
also unmaintainable -- consider how much code has to change every
time we want to add a new RTL or TREE code.

Additionally, from my experience with gcc the default cases were
omitted in the past for those cases we know are "don't care"
conditions.

Yes, there may be a few that should be aborts.  But the behavior with
the default: break case matches the current behavior -- which is
what I think we want to do unless we know its wrong (as has been
the case for some of the suggested paren warning fixes).

The goal of fixing warnings right now is to get rid of most of the
noise so that the warnings which are more likely to represent real
problems can be identified and dealt with.  The patches to use
default:break work towards that goal and (IMHO) don't make it any
more difficult to find the few cases that should be aborts.


jeff

Re: Warning elimination patches in cp/call.c

[Jeffrey A Law]

  In message < 199803101742.JAA19093@atrus.synopsys.com >you write:
  > > Yes, there may be a few that should be aborts.  But the behavior with
  > > the default: break case matches the current behavior -- which is
  > > what I think we want to do unless we know its wrong (as has been
  > > the case for some of the suggested paren warning fixes).
  > 
  > It seems that inserting default: break should be fine in most cases.  The
  > only exception is where each case computes a needed result or returns a
  > value; here default: break won't compute the result or return a value, so
  > it seems that there should be a default: abort in such cases (otherwise
  > we'll wind up with an uninitialized variable or fall off the end of
  > a function).
Yup.  In face I've already fixed a couple where a break caused us
to drop off the end of a non-void function without returning a 
value.  Of course -Wall told me I'd fallen off the end of the
non-void function :-)


jeff

Re: Warning elimination patches in cp/call.c

[Joe Buck]

> This is an enormous amount of work, not to mention flat out impossible
> since we allow language front ends to make their own tree nodes.

OK, this makes it clear that we can't expect to individually handle
every case.

> Yes, there may be a few that should be aborts.  But the behavior with
> the default: break case matches the current behavior -- which is
> what I think we want to do unless we know its wrong (as has been
> the case for some of the suggested paren warning fixes).

It seems that inserting default: break should be fine in most cases.  The
only exception is where each case computes a needed result or returns a
value; here default: break won't compute the result or return a value, so
it seems that there should be a default: abort in such cases (otherwise
we'll wind up with an uninitialized variable or fall off the end of
a function).

Re: Warning elimination patches in cp/call.c

[Craig Burley]

See gcc/f/com.c ffecom_expr_intrinsic_, the huge long switch statement,
for an example of the usefulness of avoiding `default:' in some
situations.

Here, the statement evolved to its current form for a variety of
reasons, but the main reason to avoid `default:' now is to ask
for gcc's help identifying intrinsics that have been added to the
table of intrinsics (gcc/f/intrin.def) but for which no actual
code-generation support has been added.  That avoids mistakes I've
frequently made in the past, in released versions of g77.

The cost is a "huge" number of unnecessary `case' statements.  Note
how they're grouped into two categories.  One is the `break;'
behavior, which is commented accordingly and is where the
temptation to use `default:' in place of all these can cause
subtle code-generation bugs.  (Namely, forget to put explicit
code to code-gen an intrinsic in here, and the code-gen that
happens makes assumptions that might be subtly wrong.)  So these
cases all are known (or strongly believed ;-) to be ones where
the fall-through behavior is wanted.

The other is the `die horrible death at run time' (aka `abort()')
behavior, another place tempting to use `default:', where doing
so causes less problematic results.  (Namely, forget to put
explicit code here, and the code-gen crashes the compiler.)  These
cases are known (believed) to be ones that simply shouldn't happen
in this section of code.

By avoiding `default:' entirely, the risk of subtle code-generation
bugs is still present (when compiling without asking for warnings),
but the typical case (compiling via recent gcc's with -W -Wall)
is that you get warned at *build* time, not run time compiling code
that happens to use the intrinsic in question, that the intrinsic
does not have its own code-gen code.

I believe that's a good example of when to avoid `default:',
though a better one is where the default `break;' behavior is
valid for forgotten cases, such as when finding optimal cases
to call out, for which the default behavior should still work.

Whenever the whole `switch' statement can be eliminated, e.g.
it's searching for cases to optimize, `default: break;' should
be fine.  Otherwise, some careful thought needs to go into
how to balance things like compile-time optimizations, possibility
of forgetting cases, results of forgotten cases (low or high
risk of doing bad things), and so on.

Being a bit of a language pedant, I'd love a way to specify that
the default behavior for a particular `switch' is to abort
rather than break *without* disabling compiler warnings about
unhandled cases (e.g. maybe a keyword like `missing:' in place
of `default:').  That'd be what I'd want to use in place of
what ffecom_expr_intrinsic_ currently does.  But C is not a
language for pedants.  ;-)

Actually, it occurs to me that the code in question could be
made *slightly* safer by inserting a `default:' in with the
aborting `case' statements, but conditionalizing it on being
compiled by a non-gcc compiler, so gcc continues to warn
about it, while other compilers (which might not) know to
make the program (g77, or f771 really) crash and burn rather
than fall through for missing cases.

        tq vm, (burley)

P.S. A note about Jeff Law's response: yes, it's true that
trying to be too pedantic about this stuff can be problematic,
especially when not all cases can be known to the author
of the code (e.g. where a language front-end adds more cases
via dynamic build-time configuration, true for C++ for example).

In other situations (mostly), it's important to remember that
the "cost of maintenance" of adding a new case globally (i.e.
adding a new tree node) isn't just limited to whether one
must hand-edit `switch' statements to avoid new warnings.

If those `switch' statements are properly modified *now*, then
the cost of maintenance might *appear* to be higher when new
nodes are added.

But the cost of maintenance could be *much* higher than if
we don't modify the statements correctly now, if the impact is
that we can't easily find where new `case' statements *should*
be added for, say, a new tree node, resulting in incorrect
code being generated.

In summary, among the *lowest* factors of "cost of maintenance"
is anything that requires a knowledgable programmer to analyze
and hand-edit code based on automatically produced reports
highlighting possible bugs.  Make that kind of work "go away"
by eliminating the reports, and you increase the risk of
incurring the *highest* factors of "cost of maintenance",
for example, less-knowledgable programmers trying to debug,
perhaps months later, the output of the application in question.

Maybe the most important point we're all making is this:

  Nobody should try to eliminate compiler warnings without
  knowing a *lot* about what they're doing.  If their work
  could truly be replaced via automation, then don't let them
  do it.