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

[Augie Fackler]

On Wed, Aug 19, 2015 at 10:00:42PM +0900, Yuya Nishihara wrote:
> 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?

Hm, no I don't think we need to worry about that especially.

>
> 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.

I thought this series moved from using PySet_ functions to PyList_
functions, so it'd still break old .py with new .c?