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

[Dan Carpenter]

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...

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.

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

regards,
dan carpenter