[PATCH 2 of 4 V3 part 2] ancestor: add lazy membership testing to lazyancestors
Siddharth Agarwal
sid0 at fb.com
Tue Dec 18 12:02:47 CST 2012
On 12/18/2012 08:43 AM, Kevin Bullock wrote:
> Looks good, but some comments below. I don't see any obvious code
> paths that would suffer from the latter case, do you have a sense of
> this?
Well, I guess you could hit it if you're e.g. rebasing from tip to 0,
but then you have bigger problems. In general we're almost always better
off.
> I'm uneasy about this change being included in this patch. It should
> at least be referenced in the commit message, if not moved to a
> subsequent patch.
It is referenced already in the commit message, isn't it? That's where
the perf numbers come from.
> I'd prefer if these were simply named self._seen and self._visit, with
> a comment that they aren't to be used for iteration.
I'm torn here -- that is what I originally had, but I thought that was
somewhat confusing even with a comment.
>> +def isinlazyset(s, l):
>> + print [n for n in l if n in s]
> The name of this function is entirely opaque to me. Something more like 'printlazyancestors' would be an improvement.
Yes, the name is a leftover from an early iteration. I'll fix this.
More information about the Mercurial-devel
mailing list