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

From: Junio C Hamano <junkio@cox.net>
Date: Apr 29, 2007, 06:59 UTC
Message-ID: <7vy7kbeke1.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20070427181326.14bbbf5c@localhost>

"Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br> writes:

Show 19 quoted lines

> Em Fri, 27 Apr 2007 12:32:11 -0700
> Junio C Hamano <junkio@cox.net> escreveu:
>
> | "Luiz Fernando N. Capitulino" <lcapitulino@mandriva.com.br>
> | writes:
> | 
> | > From: Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>
> | >
> | > This macro may be used to iterate over revisions, so, instead of
> | > doing: ...
> | 
> | I am not a big fan of magic control-flow macros, as it makes the
> | code harder to grok for people new to the codebase.
>
>  Yeah, I agree. But I think that any experienced programmer will
> understand it.
>
>  Anyways, I don't want to raise polemic discussions for minor
> changes. Feel free to drop this one then.

I on the other hand like the kernel style list macros.

The reason I do not like this particular one is because both operations you are hiding are not simple operations like "initialize a variable to list head" or "follow a single pointer in the structure", but rather heavyweight operations with rather complex semantics. I would want to make sure that people realize they are calling something heavyweight when they use the revision traversal.