This is the mail archive of the
gcc@gcc.gnu.org
mailing list for the GCC project.
Re: Checking patch
- To: Martin von Loewis <martin at mira dot isdn dot cs dot tu-berlin dot de>
- Subject: Re: Checking patch
- From: Jeffrey A Law <law at cygnus dot com>
- Date: Wed, 06 May 1998 20:38:28 -0600
- Cc: egcs at cygnus dot com, wilson at cygnus dot com
- Reply-To: 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