From: Neil Brown <neilb@suse.de>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org
Subject: Re: raid5: coding style cleanup / refactor
Date: Sat, 16 Jun 2007 08:39:34 +1000 [thread overview]
Message-ID: <18035.5414.680663.405523@notabene.brown> (raw)
In-Reply-To: message from Dan Williams on Tuesday June 12
Sorry for not getting to this soon...
On Tuesday June 12, dan.j.williams@intel.com wrote:
>
> From: Dan Williams <dan.j.williams@intel.com>
>
> Most of the raid5 code predates git so the coding style violations have
> been present for a long time. However, now that major new patches are
> arriving, checkpatch.pl complains about these old violations. Instead of
> attempting to justify the warnings as "this is what raid5 used to do", this
> patch brings the code in line with the current style.
I general I would rather just fix things as they break. i.e. when you
change a line that violates the style, fix it then.
And some of the changes you make, I don't like. e.g. Everything
inside parentheses should be to the right of the opening parenthesis.
>
> Note that this is more than a simple reformatting. The majority of the
> 80-column violations were in the handle_stripe5 and handle_stripe6
> routines. By introducing the 'stripe_head_state' and 'r6_state' objects
> large portions of the logic could be moved to sub-routines, reclaiming a
> column's worth of indentation.
This refactoring, however, is a good idea. We have some very deep
functions in there and breaking them out is a good idea. So a patch
that just does this would be very welcome.
>
> 'stripe_head_state' consumes all of the automatic variables that previously
> stood alone in handle_stripe. 'r6_state' contains the handle_stripe6
> specific variables like p_failed and q_failed.
>
> One of the nice side effects of the 'stripe_head_state' change is that it
> allows for further reductions in code duplication between raid5 and raid6
> (note: unfortunately the other 80-column violations hide the code
> duplication-removal effect in the diffstat). The following new routines
> are shared between raid5 and raid6:
>
> handle_completed_write_requests
> handle_requests_to_failed_array
> handle_stripe_expansion
>
> Trivia:
> * PRINTK and RAID5_DEBUG is replaced with pr_debug
Good idea... Am I asking too much to have separate things in separate
patches? It makes review easier.
> * struct field comments moved to the top (kernel-doc format)
I guess... though moving the documentation away from the code means
that it is less likely to be kept up-to-date.
NeilBrown
next prev parent reply other threads:[~2007-06-15 22:39 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-06-12 17:41 raid5: coding style cleanup / refactor Dan Williams
2007-06-12 21:25 ` Andrew Morton
2007-06-12 21:32 ` Williams, Dan J
2007-06-12 22:09 ` Dan Williams
2007-06-13 18:53 ` Dan Williams
2007-06-14 21:08 ` Bill Davidsen
2007-06-15 2:39 ` Dan Williams
2007-06-15 22:39 ` Neil Brown [this message]
2007-06-15 23:55 ` Dan Williams
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=18035.5414.680663.405523@notabene.brown \
--to=neilb@suse.de \
--cc=akpm@linux-foundation.org \
--cc=dan.j.williams@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-raid@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).