Patchwork dirs._addpath: reinstate use of Py_CLEAR

login
register
mail settings
Submitter Siddharth Agarwal
Date April 8, 2015, 3:48 a.m.
Message ID <aa415df87e26f4c193fe.1428464931@devbig136.prn2.facebook.com>
Download mbox | patch
Permalink /patch/8560/
State Accepted
Commit 67241ee427cfb457d7728e44fc772572c1555c2e
Headers show

Comments

Siddharth Agarwal - April 8, 2015, 3:48 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1428464584 25200
#      Tue Apr 07 20:43:04 2015 -0700
# Node ID aa415df87e26f4c193fe976ff0de355cfd1590cf
# Parent  54e5c239c2d9ad87fc2080fd9be35765cf0ebc9f
dirs._addpath: reinstate use of Py_CLEAR

I changed this to an explicit Py_DECREF + set to null in 6f0e6fa9fdd7. This was
a silly misunderstanding on my part -- for some reason I thought Py_CLEAR set
its argument to null only if its refcount reached 0. Turns out that's not
actually the case -- Py_CLEAR is just Py_DECREF + set to null with some
additional precautions around destructors that aren't relevant here.

The real bug that 6f0e6fa9fdd7 fixed was the fact that we were mutating the
string after setting it in the Python dictionary.
Martin von Zweigbergk - April 8, 2015, 4:09 a.m.
On Tue, Apr 7, 2015 at 8:50 PM Siddharth Agarwal <sid0@fb.com> wrote:

> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1428464584 25200
> #      Tue Apr 07 20:43:04 2015 -0700
> # Node ID aa415df87e26f4c193fe976ff0de355cfd1590cf
> # Parent  54e5c239c2d9ad87fc2080fd9be35765cf0ebc9f
> dirs._addpath: reinstate use of Py_CLEAR
>
> I changed this to an explicit Py_DECREF + set to null in 6f0e6fa9fdd7.
> This was
> a silly misunderstanding on my part -- for some reason I thought Py_CLEAR
> set
> its argument to null only if its refcount reached 0. Turns out that's not
> actually the case -- Py_CLEAR is just Py_DECREF + set to null with some
> additional precautions around destructors that aren't relevant here.
>
> The real bug that 6f0e6fa9fdd7 fixed was the fact that we were mutating the
> string after setting it in the Python dictionary.
>

That was my understanding as well. Thanks. Pushing to the clowncopter.


> diff --git a/mercurial/dirs.c b/mercurial/dirs.c
> --- a/mercurial/dirs.c
> +++ b/mercurial/dirs.c
> @@ -94,10 +94,8 @@ static int _addpath(PyObject *dirs, PyOb
>                         goto bail;
>
>                 /* Clear the key out since we've already exposed it to
> Python
> -                  and can't mutate it further. key's refcount is
> currently 2 so
> -                  we can't just use Py_CLEAR. */
> -               Py_DECREF(key);
> -               key = NULL;
> +                  and can't mutate it further. */
> +               Py_CLEAR(key);
>         }
>         ret = 0;
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/mercurial/dirs.c b/mercurial/dirs.c
--- a/mercurial/dirs.c
+++ b/mercurial/dirs.c
@@ -94,10 +94,8 @@  static int _addpath(PyObject *dirs, PyOb
 			goto bail;
 
 		/* Clear the key out since we've already exposed it to Python
-		   and can't mutate it further. key's refcount is currently 2 so
-		   we can't just use Py_CLEAR. */
-		Py_DECREF(key);
-		key = NULL;
+		   and can't mutate it further. */
+		Py_CLEAR(key);
 	}
 	ret = 0;