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.
+
+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.
+
+/*
+ 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.
/* If we've just had a newline, write the indention amount,
potentially modified.
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. When modifier is -1 the leading space into
the indention is swallowed. */
Hmm, reading it MODIFIER seems like a particularly bad NAME. Can you
come up with a better name for that parameter?
+static void
+write_any_indent (int modifier)
+{
+ int i;
+ if (had_recent_newline)
+ for (i = 0; i < indent_amount + modifier; i++)
+ fprintf (state_file, " ");
+ had_recent_newline = 0;
+}
+
+static void
+write_open_paren (const char *tag)
+{
+ write_new_line ();
+ write_any_indent (0);
+ fprintf (state_file, "(!%s ", tag);
+ indent_amount++;
+}
+
+static void
+write_close_paren (void)
+{
+ indent_amount--;
+ write_any_indent (0);
+ fprintf (state_file, ")");
+ write_new_line ();
+}
Block comments before each function. I realize they're pretty trivial,
but we really try to always have that block comment.
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.
+ 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.
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.
With those nits fixed, I think this patch will be ready. Please update
& repost for final approval.
jeff
