public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
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

  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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox