Re: [PATCH 41/47] staging/lustre/llite: remove dead code

[Dan Carpenter <dan.carpenter@oracle.com>]

On Tue, Apr 29, 2014 at 07:16:54PM +0000, Hammond, John wrote:
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Tuesday, April 29, 2014 6:03 AM
> > To: Oleg Drokin
> > Cc: Greg Kroah-Hartman; linux-kernel@vger.kernel.org;
> > devel@driverdev.osuosl.org; Drokin, Oleg; Hammond, John
> > Subject: Re: [PATCH 41/47] staging/lustre/llite: remove dead code
> > 
> > On Sun, Apr 27, 2014 at 01:07:05PM -0400, Oleg Drokin wrote:
> > > From: "John L. Hammond" <john.hammond@intel.com>
> > >
> > > In llite remove unused declarations, parameters, types, and unused,
> > > get-only, or set-only structure members. Add static and const
> > > qualifiers to declarations where possible.
> > >
> ...
> > 
> > This is a random grab bag of changes to lots of files.  One thing per
> > patch, etc, next time.
> 
> OK, granted. But some guidance would be welcome here.

No problem.  This is something a lot of people have questions about.

> For
> clean-up work like this, do you want a patch that const-corrects one
> function, a patch that const corrects all functions in a file, or a
> patch that const corrects all functions in a module?

All the functions in the module is fine.

> Is it OK to do const and static correction in the same change?

That's a borderline case.  My first instinct is to say no.

Are we talking about a single variable and making it const in one
patch and then const static in the next?  That's obviously better done
as one change.  But if you're talking about different variables, then
maybe it's better as two changes.  But then maybe some variables should
be made into "static const" and some should be just "static".

These things depend on how you sell it.

[patch] staging: lustre/llite: tighten up static and const declarations

That would be ok probably.

> Is it OK to do const, static, and dead-code in a single file?

No.

Greg compile tests patches but I normally don't.  When I review +static
patches then I pipe it to a command that strips out all the +static
changes and I verify that nothing else changed.  When I review dead code
patches I scan it for places which add code and look at why it does
that.  Then I quickly rescan to verify that the dead code really is
dead.  Normally I can tell just from looking at the patch because there
is an "#if 0" but if it's something more complicated then hopefully it's
explained in the changelog.  If you mix the two kinds of changes then it
messes up my review process.

regards,
dan carpenter