[ovs-dev] [kernel-reload 5/8] ovsdb-client: Break table formatting into new library.

Ben Pfaff blp at nicira.com
Wed Feb 9 00:09:53 UTC 2011


On Tue, Feb 08, 2011 at 01:27:48PM -0800, Ethan Jackson wrote:
> Looks Good.  Some minor comments below.
> 
> > @@ -0,0 +1,518 @@
> > +/*
> > + * Copyright (c) 2009, 2010, 2011 Nicira Networks.
> > + *
> Should this only be 2011?

This new file is extracted out of ovsdb-client.c, which had all those
copyright years, so it is my understanding that I should keep them when
I move out that code.  IANAL so I could be wrong...

> +static const char *
> +cell_to_text(const struct cell *cell_, const struct table_style *style)
> +{
> +    struct cell *cell = (struct cell *) cell_;
> 
> I don't see a reason to make cell_ const as nothing relies on it being const
> and it is not in-fact const in this context.

Fair enough, changed.

> + *     cell actually contains OVSDB data because 'text' cannot be formatted
> + *     according to the table style; it is always output verbatim.
> + */
> "output verbatim" -> "outputted verbatim"

"outputted" is really ugly.  The dictionaries I've looked in
(wiktionary, Merriam-Webster) say that "output" is also correct.

> 
> +            if (length > widths[x])
> +                widths[x] = length;
> 
> This should have braces.

Thanks, fixed.

> +struct cell *table_add_cell(struct table *table);
> I think this should be: struct cell *table_add_cell(struct table *);

Thanks, fixed.




More information about the dev mailing list