Re: [PATCH 1/5] Introduces for_each_revision() helper

From: Alex Riesen <raa.lkml@gmail.com>
Date: Apr 28, 2007, 11:50 UTC
Message-ID: <20070428115059.GA4888@steel.home>
In-Reply-To: <Pine.LNX.4.64.0704280446180.12006@racer.site>

Johannes Schindelin, Sat, Apr 28, 2007 04:46:41 +0200:

Show 9 quoted lines

> > +#define for_each_revision(commit, rev) \
> > +	for (prepare_revision_walk(rev); \
> > +		  (commit = get_revision(rev)) != NULL; )
> > +
> >  #endif
> 
> I object to this, additionally to the magic argument that I agree to, on 
> the grounds that it is actually wrong. The first iteration will work on an 
> _uninitialized_ "commit" variable.

No, it wont. Check it. This code is correct.

> Furthermore, it is not like it was a huge piece of code that is being 
> replaced by a shortcut. There are better places to do some libification 
> than this.

It is not about libification. It is plain readability issue. Look at what list_for_each_* macros did to the source of Linux kernel.