From: Greg KH <greg@kroah.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
Subject: Re: rename_rev.pl: review script for whitespace changes
Date: Fri, 26 Jun 2015 10:07:32 -0700 [thread overview]
Message-ID: <20150626170732.GA15099@kroah.com> (raw)
In-Reply-To: <20150626131524.GQ30834@mwanda>
On Fri, Jun 26, 2015 at 04:15:24PM +0300, Dan Carpenter wrote:
> I've sent my review script out a few times before but we have some new
> reviewers in staging who maybe haven't tried them.
>
> rename_rev.pl strips out whitespace changes. We recently had someone
> send a re-indent patch that deleted a line of code by mistake. The diff
> looked like:
>
> 18 files changed, 906 insertions(+), 927 deletions(-)
>
> It would be difficult to spot the bug manually but when you cat it
> through rename_rev.pl then it stands out immediately.
>
> If the patch changes // comments to /* */ then `rename_rev.pl -nc`
> strips out most of the comments.
>
> If the patch re-indents macros then the -ns removes slashes from the
> ends of lines.
>
> Sometimes people pull out some code into a separate function. The -pull
> option is supposed to help for that. It sometimes does...
>
> The other thing that we see a lot in staging is when people change curly
> braces around. The -nb option removes curly brace changes.
>
> Another thing is we had a change which did this:
>
> -#define HOST_IF_MSG_SCAN ((u16)0)
> -(40 lines of similar code)
> +#define HOST_IF_MSG_SCAN 0
> +(40 lines of similar code)
>
> I used rename_rev.pl -e 's/\(\(u16\)(.*)\)/$1/' to make sure nothing
> else changed.
>
> Or if you are making hundreds of functions "static", then I just remove
> all the statics by doing rename_rev.pl -ea 's/static//'. The -ea option
> stands for Execute on All.
>
> Oh. And I am also going to include my move_rev.pl, script. That is for
> if we move functions from one file to another.
>
> cat patch | move_rev.pl | rename_rev.pl
>
> The rename_rev.pl script is sort of crappy, but often it removes a lot
> of stuff. It doesn't have to be perfect to be better, I guess.
>
> What I wish is that there were an -auto option which would find which
> variables were renamed. Oh! Oh! I have left out the most important
> feature. Say you are renaming variables from SomeName to some_name then
> cat patch | rename_rev.pl SomeName some_name TwoName two_name Foo foo
>
> regards,
> dan carpenter
Thanks a lot for these, much appreciated, I'll work to add them to my
test scripts for patches.
greg k-h
next prev parent reply other threads:[~2015-06-26 17:07 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-26 13:15 rename_rev.pl: review script for whitespace changes Dan Carpenter
2015-06-26 17:07 ` Greg KH [this message]
2015-06-27 8:05 ` Sudip Mukherjee
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=20150626170732.GA15099@kroah.com \
--to=greg@kroah.com \
--cc=dan.carpenter@oracle.com \
--cc=devel@driverdev.osuosl.org \
--cc=linux-kernel@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.