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

"Luiz Fernando N. Capitulino" [off-list ref]
writes:

Em Fri, 27 Apr 2007 12:32:11 -0700
Junio C Hamano [off-list ref] escreveu:

| "Luiz Fernando N. Capitulino" [off-list ref]
| writes:
| 
| > From: Luiz Fernando N. Capitulino [off-list ref]
| >
| > 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.

Junio C Hamano [off-list ref] wrote:

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.

But in_merge_base is heavyweight if the two commits are in the
same object database, but aren't connected at all.  You'll need
to traverse both histories before aborting and saying there is
no merge base.  That ain't cheap on large trees.  But its also a
single line of code.

Anyway, my original problem with this macro was the way it was
defined.  I think Luiz was able to fix most of my issues with it
in his latest version, but I still have a personal distaste for
hiding things like a for(;;) construct in a macro, or allowing a
macro parameter to be used more than once within the definition of
the macro (unexpected side-effects of evaluating an more than once).

-- 
Shawn.