On Tue, Apr 29, 2014 at 07:16:54PM +0000, Hammond, John wrote: > > -----Original Message----- > > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > > Sent: Tuesday, April 29, 2014 6:03 AM > > To: Oleg Drokin > > Cc: Greg Kroah-Hartman; linux-kernel@xxxxxxxxxxxxxxx; > > devel@xxxxxxxxxxxxxxxxxxxx; 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@xxxxxxxxx> > > > > > > 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
