On Wed, Sep 24, 2014 at 03:01:58PM +0300, Dan Carpenter wrote:
>
> 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.
Hi Dan,
I never knew about that vim feature, thanks.
>
> >
> > 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.
Ok. I see your point.
>
> 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++;
> }
Fair point, I've simplified this loop in a patch.
There are also a lot of comments that are either out of date or incorrect. I've
also attempted to address this with another patch.
Cheers,
Mark
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
