Use indentation in gtype.state to show nested structure

On Mon, 2013-05-06 at 12:22 -0600, Jeff Law wrote:
> On 05/03/2013 06:48 PM, David Malcolm wrote:
> >
> > +static int indent_amount = 0;
> > +static int had_recent_newline = 0;
> Any clean way to do this without the global state?  I have to ask given 
> you're generally working on removing global state.  Seems to take a tiny 
> step backwards.

Note that this code is for the gengtype build-time utility, rather than
in GCC proper, so this wasn't on my own mental hitlist for fixing global
state.

The attached rewrite of the patch introduces classes to hold the new
internal state relating to writing out the s-expressions, and by doing
so implicitly adds a requirement that this code be compiled with C++
(I'm not sure that such a requirement existed before).

The new variables become data members of a new s_expr_writer class (and
as such gain trailing underscores as per
http://gcc.gnu.org/codingconventions.html#Cxx_Names )

It does not remove all global state from the writer code, merely the new
state that the old version of the patch added.  For example:
  FILE * state_file
is still global (and shared by the reader code), but fixing that would
make the patch touch many more lines of code and thus be more difficult
to read.

> > +
> > +static void
> > +write_new_line (void)
> > +{
> > +  /* don't add a newline if we've just had one */
> > +  if (!had_recent_newline)
> > +    {
> > +      fprintf (state_file, "\n");
> > +      had_recent_newline = 1;
> > +    }
> > +}
> Need a block comment before WRITE_NEW_LINE.

Fixed (now s_expr_writer::write_new_line)

> > +
> > +/*
> > +  If we've just had a newline, write the indentation amount,
> > +  potentially modified.
> > +
> > +  The modifier exists to support code that writes strings with leading
> > +  space (e.g " foo") which might occur within a line, or could be the first
> > +  thing on a line.  By passing a modifier of -1, when such a string is the
> > +  first thing on a line, the modifier swallows the leading space into the
> > +  indentation, and all the fields line up correctly.
> > +*/
> Comment style is goofy.  Something like this seems better.  Note the 
> placement of the start/end comment markers and use of caps when 
> describing a variable or parameter name.

Fixed.

> Hmm, reading it MODIFIER seems like a particularly bad NAME.  Can you 
> come up with a better name for that parameter?

I changed this to "leading_spaces", and changed the sign of it, so all
the calls with -1 became calls with 1.

> Block comments before each function.  I realize they're pretty trivial, 
> but we really try to always have that block comment.
Fixed - though some of them are very trivial (e.g. for constructors)

> It's probably worth noting that these also insert newlines since it's 
> not trivial to determine simply by looking at the function name. 
> Similarly for the open-paren.  It's actually open-paren-!.  Closing 
> paren seems to be closing-paren + newline.  Not sure if it's worth 
> renaming though.  Just thought I'd point it out since I had to 
> double-check when reviewing the later code.

I renamed these to begin_s_expr () and end_s_expr () in the new version
of the patch to make the higher-level intent more clear.

> > +  write_close_paren (); /* (!fields */
> We don't generally write comments on the end of lines like this. 
> Belongs on the line before and generally should be written as a complete 
> sentence.  This occurs often and needs to be fixed.

I removed most of these comments, except for the cases where the
begin/end pairs were widely separated, or where there could be different
choices of kind of s-expr.  I rewrote these comments as per your review.

> Please check the indention on your changes themselves. 
> write_state_options & write_state_lang_bitmap look like they got mucked 
> up, though I didn't look terribly closely.

I believe I already fixed these in
http://gcc.gnu.org/ml/gcc-patches/2013-05/msg00259.html 

(again, is there an automated way of checking this?)

> With those nits fixed, I think this patch will be ready.  Please update 
> & repost for final approval.

Thanks for review; see attached rewrite of patch.

Dave

Use indentation in gtype.state to show nested structure

Commit Message

Comments

Patch