[PATCH 5 of 7] reachableroots: use internal "revstates" array to test if rev is a root

[Yuya Nishihara]

On Tue, 18 Aug 2015 15:44:35 -0400, Augie Fackler wrote:
> On Tue, Aug 18, 2015 at 12:32:55PM -0700, Pierre-Yves David wrote:
> >
> >
> > On 08/18/2015 12:26 PM, Pierre-Yves David wrote:
> > >
> > >
> > >On 08/18/2015 07:42 AM, Yuya Nishihara wrote:
> > >># HG changeset patch
> > >># User Yuya Nishihara <yuya at tcha.org>
> > >># Date 1439534609 -32400
> > >>#      Fri Aug 14 15:43:29 2015 +0900
> > >># Node ID 93e5cd30d2ff9b63f11616054cb647d6ac998053
> > >># Parent  2ca0b48b6de1c79bc205e7a660a5531c125cad9e
> > >>reachableroots: use internal "revstates" array to test if rev is a root
> > >>
> > >>The main goal of this patch series is to reduce the use of PyXxx()
> > >>function
> > >>that is likely to require ugly error handling and inc/decref. Plus,
> > >>this is
> > >>faster than using PySet_Contains().
> > >>
> > >>   revset #0: 0::tip
> > >>   0) 0.004168
> > >>   1) 0.003678  88%
> > >>
> > >>This patch ignores out-of-range roots as they are in the pure
> > >>implementation.
> > >>Because reachable sets are calculated from heads, and out-of-range
> > >>heads raise
> > >>IndexError, we can just take out-of-range roots as unreachable.
> > >>Otherwise,
> > >>the test of "hg log -Gr '. + wdir()'" would fail.
> > >>
> > >>"heads" argument is changed to a list. Should we have to rename the C
> > >>function
> > >>as its signature is changed?
> > >
> > >Do we actually need (take advantage of) theses argument being a list?
> > >Could it take an arbitrary iterable? This would allow use to pass
> > >arbitrary smartset.
> >
> > So reading more of the current implementation, it seems we are not doing
> > anything specific with the list (beside iterating over it) and nothing
> > special with the set beside membership (removed in this series).
> >
> > So, I think we should enforce the argument to be smartsets and use smartset
> > available API in the C code.

Good point. Perhaps the only requirement is it should be an iterable.
Bad thing is PyIter API needs Py_DECREFs.

> > that would probably justify a signature change, but we never released this
> > code, so I'm unsure. If the old/new code raise proper exception when passed
> > the new/argument and fallback to the pure implementation, I do not think we
> > need to change the name
> 
> It will not fall back to the old version - it will just explode
> because of type mismatches. That's by design - a previous incarnation
> of the patch series silently swallowed errors and it meant that it was
> too easy to have a trivial programming error cause the C extension to
> go unused silently.

Do we have to care for the case of new .py + old .c?

New C function will be more permissive because sets and lists are iterable,
but if new .py passes a smartset to old C function, it would cause TypeError.