[REVIEW REQUEST] staging: unisys: review request for visorbus

Tobin C. Harding me at tobin.cc
Mon Nov 6 20:41:01 UTC 2017


On Mon, Nov 06, 2017 at 04:56:40PM +0300, Dan Carpenter wrote:
> On Mon, Nov 06, 2017 at 07:17:11PM +1100, Tobin C. Harding wrote:
> > > > @@ -260,9 +261,10 @@ int visorchannel_signalremove(struct visorchannel *channel, u32 queue,
> > > >  			      void *msg)
> > > >  {
> > > >  	int rc;
> > > > -	unsigned long flags;
> > > >  
> > > >  	if (channel->needs_lock) {
> > > > +		unsigned long flags;
> > > > +
> > > 
> > > Same here, the original code is fine.
> > > 
> > > No idea why the tool wants you to move these around, you should ignore
> > > stuff like that :(
> > 
> > Oh? I'm surprise at this comment. I have always thought limiting scope
> > of local variables was seen as a good thing. Is this a style thing that
> > is on case by case basis or do you never like to declare local variables
> > within blocks?
> > 
> 
> Greg has answered for himself but here are my thoughts...

thanks for taking the time.

> If you look at Colin King's patches, he's using CPPcheck and he's pretty
> religiously moving variables to the localest scope and no one complains.
> It makes sense when he does it from what I have seen.  But mostly he's
> definitely cleaning up the code and fixing bugs and no one cares too
> much about the scope one way or the other.
> 
> But here, I agreed with Greg that the original code was better.  The
> chdr_size and copy_size variables are closely related and are better
> declared together.  The flags declaration was also slightly cleaner at
> the start instead of mixing it up with the code.  Sometimes in a long
> function it definitely makes sense to use a local declaration.  So it's
> a case by case thing.

Cool, got it.

> But mostly no one cares, and I don't want to see hundreds of patches
> mechanically moving declarations around.

Oh bother, scrap that 400 patch series I queued up ... just kidding. 

thanks,
Tobin.


More information about the devel mailing list