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.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
