Re: Checking patch

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

  In message <199805022201.AAA00799@mira.isdn.cs.tu-berlin.de>you write:
  > Hi Jeff,
  > 
  > This is the final(?) version of the checking patch. It helps detecting
  > bugs in gcc when --enable-checking is given to configure, and has no
  > effect when it is not given.
  > 
  > When enabled, gcc builds and passes most of the testsuite. g++ dies
  > when compiling libgcc2. There is one invalid check in it:
  > CST_OR_CONSTRUCTOR_CHECK should really be CST_CHECK only, and the
  > processing of CONSTRUCTORs should be changed.
  > 
  > I expect another revision of this code once we are starting to fix the
  > bugs in g++.
  > 
  > If you reject this patch, please let me know why.
Cool!

First note -- I don't see a copyright assignment for gcc on record
with the FSF.   I see you've submitted an assignment for libg++, any
objection to also submitting one for gcc (I think this change is
probably large enough to warrant an assignment).

Second note -- this should have been sent to the list :-)  That
way everyone gets to look at it.  Some folks can even start playing
with it while we work out the final details.  I don't remember if
an earlier version went to the list or not.

+
+ #tree-check.h does not depend on gencheck, so that gencheck will
+ #is not built until tree.def changes
+ $(srcdir)/tree-check.h:       tree.def gencheck.c
+       $(MAKE) gencheck
+       gencheck >$(srcdir)/tree-check.h
+
+ gencheck: gencheck.c

Seems to me we should handle this in a manner similar to the
other gen* programs.  Is there some particular reason you can't
use something like this?

tree-check.h: s-check ; @true
s-check : tree.def gencheck $(srcdir)/move-if-change
        ./gencheck tree.def > tmp-check.h
        $(srcdir)/move-if-change tmp-check.h tree-check.h
        touch s-check

gencheck : gencheck.o tree.h tree.def
        $(HOST_CC) $(HOST_CFLAGS) $(HOST_LDFLAGS) -o $@ \
          gencheck.o $(HOST_LIBS)

We'll probably need to make minor tweaks to gencheck.c to deal
with system.h, toplev.h and similar stuff.  I haven't sat down
and looked at the latest round of changes, but given the simplicity
of gencheck is should be pretty trivial to bring it up to standards
re: system.h.

Most of the other problems are formatting issues.

+ tree_check(node, code, file, line, nofatal)

Always put a space before an open paren, except when defining
a macro with arguments or when it's immediately preceeded by
another open paren.  They're missing in many places, particularly
in function definitions.  They're also missing in macro bodies.

Always put a space after a comma.  Here's an example:

+ #define DO_CHECK(FUNC,t,param)   FUNC(t,param,__FILE__,__LINE__,0)

Should instead be:

+ #define DO_CHECK(FUNC, t, param)   FUNC (t, param, __FILE__, __LINE__, 0)


Most of your macros have this problem.

I see some things that aren't checked (EXPR_WFL_* stuff for example);
is there some reason why?

Why do we have the *_CHECK1 variants?

gencheck.c uses an ANSI concatenation character (#).  GCC must be
compilable by both ANSI and non-ANSI compilers, so that has to
be written to support '#' concat and the old /**/ concat method.
We may already have some checks/macros to do this.  It might be
worth a quick browse through the sources to check.

You should put whitespace around most binary operators.  ie

i=0;

should be

i = 0;

or 

i=j+2

should be
i = j + 2

I didn't see many instances of this problem.  The only one I
remember specifically is the for loop in gencheck.c.


Overall I think it's an excellent patch.  Most of the stuff I
mentioned should be relatively simple to fix.  The biggest hurdle
I see is getting a gcc assignment on file.

jeff