Re: Code review requested

Newsgroups	comp.lang.c
Date	2012-09-20 08:30 -0700
References	<5eb6a1dd-71f6-45d0-8ac9-9d4a2f3c6724@googlegroups.com> <0.fe11e41ad28c722214fc.20120920114453BST.87392d6kii.fsf@bsb.me.uk>
Message-ID	<92123780-8ef0-48b3-b237-58f4e706f5da@googlegroups.com> (permalink)
Subject	Re: Code review requested
From	Bart Vandewoestyne <bart.vandewoestyne@gmail.com>

Show all headers | View raw

On Thursday, September 20, 2012 12:44:55 PM UTC+2, Ben Bacarisse wrote:
> [...]
> A couple of points: I'd strive for more consistency in layout.  I see
> space around function arguments in some places and not others (f( x ) vs
> f(x)) and I see some operators surrounded by space and not others.  You
> have both switch( x ) and switch (x) in the same source file.  It's not
> a big point, but I find it a little distracting.

I totally agree!

> You use TRUE and FALSE for functions that return bool.  The header that
> defines bool also defined true and false, so why use your own?

The book has a

#define TRUE 1
#define FALSE 0

in util.h.  Do you mean that is not necessary because TRUE and FALSE are defined in a standard header?

> Slightly more significant is that I was puzzled by why count_exp can
> never return zero.  This might well be correct, but it's the kind of
> things that merits a comment.  I think there is a connection between
> this and the next point.

My educated guess is that it's related to the fact that for the simple language described in the book, the print statement always has at least one argument.

> My only big comment...  A lot of the code is boilerplate to deal with
> a large number of structure types (well, unions within a structure).  I
> would probably search for some way to simplify this.  I don't know the
> book, so it's possible you have to do it this way.

Indeed.  A lot of this is code taken from the book... so I try to stick with that for now...

> One big example of this that the code seems to have two kinds of
> expression list -- one used only for the last node.  This look peculiar
> to me and does seem to add complexity.  Is there some benefit to it?
> Does the book require it?  Have I misunderstood?

The grammar of the simple language described in the book also has this.  So yes, the book requires it.

> [...]
> My preference is to put the redundant return in place, preceded by an
> assert.  Then I prepare to be amazed at how often I see the assert fire!

Using assert(0) at 'unreachable' places indeed seemed like the best and cleanest solution to me.  It makes the warnings go away and on top of that, i get extra debugging facilities.

New version is online at

https://github.com/BartVandewoestyne/c/tree/master/books/Modern_Compiler_Implementation_in_C/chap01

Comments still welcome!  If no more comments, up to Chapter 2! :-)

Regards,
Bart

Back to comp.lang.c | Previous | Next — Previous in thread | Next in thread | Find similar

Thread

Code review requested bart.vandewoestyne@gmail.com - 2012-09-20 02:27 -0700
  Re: Code review requested Ben Bacarisse <ben.usenet@bsb.me.uk> - 2012-09-20 11:44 +0100
    Re: Code review requested Bart Vandewoestyne <bart.vandewoestyne@gmail.com> - 2012-09-20 08:30 -0700
      Re: Code review requested Ben Bacarisse <ben.usenet@bsb.me.uk> - 2012-09-20 16:48 +0100
        Re: Code review requested Bart Vandewoestyne <bart.vandewoestyne@gmail.com> - 2012-09-20 13:38 -0700
        Re: Code review requested Bart Vandewoestyne <MyFirstName.MyLastName@telenet.be> - 2012-09-20 23:00 +0200
      Re: Code review requested ImpalerCore <jadill33@gmail.com> - 2012-09-20 09:02 -0700
        Re: Code review requested Bart Vandewoestyne <MyFirstName.MyLastName@telenet.be> - 2012-09-20 23:13 +0200
          Re: Code review requested Jorgen Grahn <grahn+nntp@snipabacken.se> - 2012-09-22 22:16 +0000
        Re: Code review requested Keith Thompson <kst-u@mib.org> - 2012-09-20 16:54 -0700
          Re: Code review requested ImpalerCore <jadill33@gmail.com> - 2012-09-21 12:10 -0700
          Re: Code review requested David Brown <david@westcontrol.removethisbit.com> - 2012-09-23 12:47 +0200
            Re: Code review requested Keith Thompson <kst-u@mib.org> - 2012-09-23 03:54 -0700
            Re: Code review requested "BartC" <bc@freeuk.com> - 2012-09-23 14:07 +0100
            Re: Code review requested Öö Tiib <ootiib@hot.ee> - 2012-09-23 15:13 -0700
              Re: Code review requested Keith Thompson <kst-u@mib.org> - 2012-09-23 15:39 -0700
                Re: Code review requested Ian Collins <ian-news@hotmail.com> - 2012-09-24 10:46 +1200
                Re: Code review requested Keith Thompson <kst-u@mib.org> - 2012-09-23 17:30 -0700
                Re: Code review requested Ian Collins <ian-news@hotmail.com> - 2012-09-24 14:05 +1200
                Re: Code review requested Keith Thompson <kst-u@mib.org> - 2012-09-24 00:26 -0700
                Re: Code review requested David Brown <david@westcontrol.removethisbit.com> - 2012-09-24 11:03 +0200
                Re: Code review requested Nick Keighley <nick_keighley_nospam@hotmail.com> - 2012-09-24 09:16 -0700
                Re: Code review requested David Brown <david@westcontrol.removethisbit.com> - 2012-09-24 18:25 +0200
                Re: Code review requested Nick Keighley <nick_keighley_nospam@hotmail.com> - 2012-09-25 04:49 -0700
                Re: Code review requested Ian Collins <ian-news@hotmail.com> - 2012-09-25 07:41 +1200
                Re: Code review requested James Kuyper <jameskuyper@verizon.net> - 2012-09-24 15:50 -0400
                Re: Code review requested Ian Collins <ian-news@hotmail.com> - 2012-09-25 09:30 +1200
                Re: Code review requested Keith Thompson <kst-u@mib.org> - 2012-09-24 12:45 -0700
                Re: Code review requested Keith Thompson <kst-u@mib.org> - 2012-09-24 16:10 -0700
                Re: Code review requested David Brown <david@westcontrol.removethisbit.com> - 2012-09-25 09:02 +0200
                Re: Code review requested "BartC" <bc@freeuk.com> - 2012-09-24 10:47 +0100
                Re: Code review requested David Brown <david@westcontrol.removethisbit.com> - 2012-09-24 13:41 +0200
                Re: Code review requested "BartC" <bc@freeuk.com> - 2012-09-24 13:40 +0100
                Re: Code review requested David Brown <david@westcontrol.removethisbit.com> - 2012-09-24 15:31 +0200
                Re: Code review requested Ben Bacarisse <ben.usenet@bsb.me.uk> - 2012-09-24 15:14 +0100
                Re: Code review requested David Brown <david@westcontrol.removethisbit.com> - 2012-09-24 17:40 +0200
                Re: Code review requested Ben Bacarisse <ben.usenet@bsb.me.uk> - 2012-09-24 18:11 +0100
                Re: Code review requested Malcolm McLean <malcolm.mclean5@btinternet.com> - 2012-09-24 11:13 -0700
                Re: Code review requested Ben Bacarisse <ben.usenet@bsb.me.uk> - 2012-09-24 20:52 +0100
                Re: Code review requested "BartC" <bc@freeuk.com> - 2012-09-24 16:22 +0100
                Re: Code review requested Rui Maciel <rui.maciel@gmail.com> - 2012-09-24 16:46 +0100
                Re: Code review requested David Brown <david@westcontrol.removethisbit.com> - 2012-09-24 18:22 +0200
                Re: Code review requested Keith Thompson <kst-u@mib.org> - 2012-09-24 12:52 -0700
                Re: Code review requested Keith Thompson <kst-u@mib.org> - 2012-09-24 13:59 -0700
                Re: Code review requested David Brown <david@westcontrol.removethisbit.com> - 2012-09-24 08:43 +0200
                Re: Code review requested Malcolm McLean <malcolm.mclean5@btinternet.com> - 2012-09-23 16:03 -0700
                Re: Code review requested Ian Collins <ian-news@hotmail.com> - 2012-09-24 11:10 +1200
        Re: Code review requested Nick Keighley <nick_keighley_nospam@hotmail.com> - 2012-09-24 00:49 -0700
      Re: Code review requested Ian Collins <ian-news@hotmail.com> - 2012-09-21 07:19 +1200
        Re: Code review requested Bart Vandewoestyne <bart.vandewoestyne@gmail.com> - 2012-09-20 13:29 -0700
          Re: Code review requested ImpalerCore <jadill33@gmail.com> - 2012-09-20 13:48 -0700
            Re: Code review requested James Kuyper <jameskuyper@verizon.net> - 2012-09-20 17:09 -0400
              Re: Code review requested ImpalerCore <jadill33@gmail.com> - 2012-09-20 14:52 -0700
                Re: Code review requested James Kuyper <jameskuyper@verizon.net> - 2012-09-20 18:48 -0400
                Re: Code review requested Francois Grieu <fgrieu@gmail.com> - 2012-09-21 09:53 +0200
                Re: Code review requested Walter Banks <walter@bytecraft.com> - 2012-10-01 07:43 -0400
                Re: Code review requested ImpalerCore <jadill33@gmail.com> - 2012-09-21 08:55 -0700
          Re: Code review requested Eric Sosman <esosman@ieee-dot-org.invalid> - 2012-09-20 16:59 -0400
            Re: Code review requested hormelfree@gmail.com - 2012-09-20 17:39 -0700
          Re: Code review requested Rui Maciel <rui.maciel@gmail.com> - 2012-09-20 23:23 +0100
            Re: Code review requested Keith Thompson <kst-u@mib.org> - 2012-09-20 16:48 -0700

csiph-web