Excerpts from Gregory Szorc's message of 2016-11-01 19:47:07 -0700: > 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
The convert is not free (coverting from node -> rev requires walking through the whole index for the first 2 times). This patch is all about avoiding converting from node -> rev -> node (commonancestorsheads), and the return value is a bool (isancestor). Comparing with converting between node and rev, I think "isinstance" is a cheaper operation. It should be even much faster comparing with commonancestorsheads's C implementation. When the commonancestorsheads C implementation is the bottleneck, I don't think it's necessary to optimize those "isinstance" out is necessary. > `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