On Wed, Sep 24, 2014 at 11:23:13AM +0100, Mark Einon wrote:
> On Wed, Sep 24, 2014 at 01:11:42PM +0300, Dan Carpenter wrote:
> > On Mon, Sep 22, 2014 at 08:58:14PM +0100, Mark Einon wrote:
> > > static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter)
> > > {
> > > u8 id;
> > > - u32 index;
> > > + u32 ii;
> >
> > Just use "int i;". Making everything a u32 is a bad habbit and the
> > second 'i' character doesn't seem to stand for anything.
>
> Hi Dan,
>
> THanks for the review.
>
> I use 'ii' because it's easier to search in the code.
In vim you can just use the '*' key to search for a variable. The extra
"i" really needs to mean somethingi, it shouldn't just be there because
to work around a bad editor. Also there are lots of "mii_" names in
this file so I'm not sure it really helps the much for searching.
>
> It's a u32 because it's counting over fbr->num_entries, which is also
> u32, why would you change it to an int, exactly?
What I'm saying is that it's a bad habbit to use more complicated
datatypes than needed. It's not specific to this, it's throughout.
Datatypes are supposed to mean something. It means something when
someone chooses to use "u32" vs "unsigned int", it's not just because
u32 is faster to type.
By default if you have a for loop counter it should just be:
int i;
If it's not that, there should be some reason. Perhaps the index
doesn't fit in an int or perhaps the name "i" is not descriptive enough.
For example look at et131x_init_send().
u32 ct;
>From reading that I would think that "ct" stands for "count". The u32
probably means we are reading the count from the hardware because it is
32 bits (otherwise you would have use "int" or "unsigned int"). But
actually that's completely wrong information when you read the code.
Unrelated, but that loop in et131x_init_send is ugly.
1879 /* Go through and set up each TCB */
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Obvious comment just distracts.
1880 for (ct = 0; ct++ < NUM_TCB; tcb++)
^^ ^^^^^
We're counting from 1 to 64 when the overwhelming majority of C code
counts from 0-63. "tcb++" looks almost the same as "ct++" when you're
expecting to see "ct++" as the post iterator statement.
1881 /* Set the link pointer in HW TCB to the next TCB in the
1882 * chain
What does that even mean? The code is easier to understand without the
comment.
1883 */
1884 tcb->next = tcb + 1;
Multi-line indents get curly braces for readability. This loop should
look like.
for (i = 0; i < NUM_TCB; i++) {
tcb->next = tcb + 1;
tcb++;
}
regards,
dan carpenter
PS: People like to think that making everything unsigned will eliminate
bugs but my experience is that it causes a lot of signedness bugs.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
