On Mar 07, 2005 10:26 +0100, Edgar, Bob wrote:
> I lurk on the list and didn't comment last time but there is one aspect
> of this patch that I think is "bad" style. The function declaration should
> not be on the same line with the type. Why? Try to find the file where a
> function is defined instead of used. If you grep "^funcname" you'll find
> it quite simply. The same is true in YFE (mine being vi) /^funcname gets
> me there in one shot.
>
> This may not seem an important thing but when you are coming into a
> project cold and don't know how anything works or where it lives it
> can be very important. Consider trying to find where some common
> function from a library is defined in a project with sever 1000 files.
Tags is your friend. See "make tags" (for vim) or "make TAGS" (for *emacs).
This is far more efficient than "grep -r ^funcname linux" if you don't even
know what file a function/struct is defined in. Use CTRL-] to jump to the
function/struct under the cursor and CTRL-T to pop back out.
Ironically, the whitespace patch gets the small things right, but misses
on the big readability issues, such as cifs_open() being 220 lines long
and having a _really_ hard time staying inside 80 columns because of so
many levels of nested conditionals.
Judicious use of gotos and some helper functions would help a lot
here (e.g. after CIFSSMBOpen() "if (rc) { ... goto out; }" and
"if (!file->private_data) goto out;", would avoid indenting the rest
of the function 16 columns. Adding a couple helper functions like
"cifs_convert_flags()" to return desiredAccess and disposition, and
"cifs_init_private_data()" to allocate ->private_data and initialize
the masses of fields would be good.
Is it possible that pCifsInode can ever be NULL??? Similarly, "if (buf)"
on line 196 is needless, as it has already been checked on line 153
(and we abort in that case). Also, kfree() can handle NULL pointers.
Cheers, Andreas
--
Andreas Dilger
http://members.shaw.ca/adilger/ http://members.shaw.ca/golinux/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
