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

[Andy Whitcroft <apw@shadowen.org>]

Luiz Fernando N Capitulino wrote:
> This macro may be used to iterate over revisions, so, instead of
> doing:
> 
> 	struct commit *commit;
> 
> 	...
> 
> 	prepare_revision_walk(rev);
> 	while ((commit = get_revision(rev)) != NULL) {
> 		...
> 	}
> 
> New code should use:
> 
> 	struct commit *commit;
> 
> 	...
> 
> 	for_each_revision(commit, rev) {
> 		...
> 	}
> 
>  The only disadvantage is that it's something magical, and the fact that
> it returns a struct commit is not obvious.
> 
>  On the other hand it's documented, has the advantage of making the walking
> through revisions easier and can save some lines of code.
> 
> Signed-off-by: Luiz Fernando N Capitulino <lcapitulino@mandriva.com.br>
> ---
>  revision.h |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/revision.h b/revision.h
> index cdf94ad..bb6f475 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -133,4 +133,15 @@ extern void add_object(struct object *obj,
>  extern void add_pending_object(struct rev_info *revs, struct object *obj, const char *name);
>  extern void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode);
>  
> +/* helpers */
> +
> +/**
> + * for_each_revision	-	iterate over revisions
> + * @commit:	pointer to a commit object returned for each iteration
> + * @rev:	revision pointer
> + */
> +#define for_each_revision(commit, rev) \
> +	prepare_revision_walk(rev);    \
> +	while ((commit = get_revision(rev)) != NULL)
> +
>  #endif

If this is constructed like that then I would expect the code below to
be miss-compiled:

	if (condition)
		for_each_revision(commit, rev) {
		}

As it would be effectivly be:

	if (condition)
		prepare_revision_walk(rev);
	while ((commit = get_revision(rev)) != NULL) {
	}

I think you'd want this to be something more like:

#define for_each_revision(commit, rev) \
	for (prepare_revision_walk(rev); \
		(commit = get_revision(rev))) != NULL); ) {

-apw