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

Ethan Jackson ethan at nicira.com
Wed Feb 9 00:11:03 UTC 2011


Sounds good.

Ethan

On Tue, Feb 8, 2011 at 4:09 PM, Ben Pfaff <blp at nicira.com> wrote:
> 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