[PATCH 1 of 2] phase: compute phases in C
Bryan O'Sullivan
bos at serpentine.com
Fri Mar 20 17:14:20 CDT 2015
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>
More information about the Mercurial-devel
mailing list