[PATCH 4 of 6 V2] revsbetween: add a C implementation

[Laurent Charignon]

> On Aug 7, 2015, at 7:42 AM, Yuya Nishihara <yuya at tcha.org> wrote:
> 
> On Fri, 7 Aug 2015 02:28:21 -0700, Laurent Charignon wrote:
>> # HG changeset patch
>> # User Laurent Charignon <lcharignon at fb.com>
>> # Date 1438921725 25200
>> #      Thu Aug 06 21:28:45 2015 -0700
>> # Branch stable
>> # Node ID 9156d8c324dbaece372df259cb697ad15f10746b
>> # Parent  4d1b9f4def5070ac3a09dbeb396b745237a49417
>> revsbetween: add a C implementation
> 
> Just nitpicking...
> 
>> +	/* Initialize return set */
>> +	reachable = PySet_New(0);
> 
> Maybe we should use NULL consistently instead of 0?

Let's do that in a later patch series if that's okay with you

> 
>> +	if (reachable == NULL) {
>> +		PyErr_SetString(PyExc_ValueError, "memory allocation error");
>> +		goto bail;
>> +	}
> 
> IIRC, PyErr_SetString() isn't necessary here. If PyXXX() returns NULL, the
> corresponding exception would be set to the current state object.

Yes, I checked and that seems right, I will drop that line.
> 
>> +	/* Initialize internal datastructures */
>> +	tovisit = (long *)malloc((len + 1) * sizeof(long));
> 
> Do we need long width for an array of revisions?

No we don't! I will switch to int.

> 
>> +	if (tovisit == NULL) {
>> +		PyErr_SetString(PyExc_ValueError, "memory allocation error");
> 
> PyExc_MemoryError will be more appropriate.

Yes
> 
>> +release_seen:
>> +	if (seen != NULL)
>> +		free(seen);
> 
> free(NULL) is noop so long as the libc complies with the standard.

Ok