On Tue, Nov 1, 2016 at 7:25 PM, Jun Wu <qu...@fb.com> wrote:
> Excerpts from Pierre-Yves David's message of 2016-11-02 00:05:32 +0100:
> > I think I would rather see a two methods, one for revs and one for nodes
> > that would keep the signature of each function clearer. One of the
> > function would probably all into the other one to keep the code common.
> >
> > What do you think?
>
> If we have two "commonancestorsheads"s, we will have to write two
> "isancestor"s. I'd avoid the contagious pattern that enforces the caller
> ("isancestor" in this case) to do things otherwise unnecessary.
>
> Given the fact that "repo.rev(x)" takes both node and rev. And repo[x]
> takes
> everything. I'd prefer shorter code. It can be seen as a reasonable
> function
> overloading in C++.
>
I agree with Pierre-Yves here: it is too easy to fall into performance
traps where functions accept either node or rev and needlessly perform type
checking or dip into the index for unneeded lookups. Let's standardize on
one. With list comprehensions, it's easy enough to ensure arguments are
either all revs or all nodes.
I do disagree about the need for 2 methods. Just standardize on whatever
one makes sense and have the caller convert to that type. This may entail a
`revlog.revstonodes(revs)` or `revlog.nodestorevs(nodes)` to facilitate
bulk converting iterables of 1 type to the other.
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
