[PATCH 1 of 2] phase: compute phases in C

[Bryan O'Sullivan]

On Fri, Mar 20, 2015 at 11:20 AM, Laurent Charignon <lcharignon at fb.com>
wrote:

> phase: compute phases in C
>

Some commentary below.


> +               if (iter == NULL) {
> +                       return -2;
> +               }
>

We generally don't wrap one-line blocks in curly braces.


> +                       if (iter_item_long < min_idx) {
> +                               min_idx = iter_item_long;
> +                       }
>

See above.


> +       } else {
> +               return index_length(self);
> +       }
>

Drop the enclosing else block and just use a plain return here.


> +       if ((parent_1 >= 0 && phases[parent_1] > phases[i])) {
> +               phases[i] = phases[parent_1];
> +       }
> +       if ((parent_2 >= 0 && phases[parent_2] > phases[i])) {
> +               phases[i] = phases[parent_2];
> +       }
>

See above about single-line curlies (I'm going to stop pointing this out).


> +       /* Argument validation */
>

Unnecessary comment :-)


> +       phases = calloc(len, 1); /* phase per rev: {0: public, 1: draft,
> 2: secret} */
>

Unchecked memory allocation needs a check.


> +       /* Transform phase list to a python list */
> +       PyObject *phaseslist = PyList_New(len);
> +       Py_INCREF(phaseslist);
>

Why are you incrementing the refcount to 2 here? This doesn't make obvious
sense.

A general comment: the cleanup management in this function is quite
difficult to understand due to the multiple exits. I don't have time to
reason through it to trust that it's correct.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20150320/0339ea23/attachment.html>