| Submitter | Kevin Bullock |
|---|---|
| Date | June 12, 2014, 3:53 a.m. |
| Message ID | <79CD695C-F509-41FA-9A64-36A8B8872E56@ringworld.org> |
| Download | mbox | patch |
| Permalink | /patch/4981/ |
| State | Superseded |
| Commit | 8da100383dc32c1f12dcc3ce8dd4ea38c6281158 |
| Headers | show |
Comments
On 6/11/2014 9:44 PM, Sean Farley wrote: > > Kevin Bullock writes: > >> On Jun 11, 2014, at 5:43 PM, danek.duvall@oracle.com wrote: >> >>> # HG changeset patch >>> # User Danek Duvall <danek.duvall@oracle.com> >>> # Date 1402525864 25200 >>> # Wed Jun 11 15:31:04 2014 -0700 >>> # Branch stable >>> # Node ID b091b262aa6d25e7e4663672daf2b6d8f127ab48 >>> # Parent b35f8c487e396487e89f98e92da57ac5eb9833af >>> parsers.c: fix a couple of memory leaks >>> >>> diff --git a/mercurial/parsers.c b/mercurial/parsers.c >>> --- a/mercurial/parsers.c >>> +++ b/mercurial/parsers.c >>> @@ -1403,8 +1403,12 @@ static PyObject *find_deepest(indexObjec >>> final |= i; >>> j -= 1; >>> } >>> - if (final == 0) >>> + if (final == 0) { >>> + free(depth); >>> + free(seen); >>> + free(interesting); >>> return PyList_New(0); >>> + } >> >> Good catch, but this is now the 3rd place we've got those same `free()`s in this function. Would something like this work instead: >> >> diff --git a/mercurial/parsers.c b/mercurial/parsers.c >> --- a/mercurial/parsers.c >> +++ b/mercurial/parsers.c >> @@ -1403,8 +1403,10 @@ static PyObject *find_deepest(indexObjec >> final |= i; >> j -= 1; >> } >> - if (final == 0) >> - return PyList_New(0); >> + if (final == 0) { >> + keys = PyList_New(0); >> + goto cleanup; > > http://xkcd.com/292/ I support the use of goto in C for intra-function jumping to cleanup code. The alternatives are duplicated code (leading to sync issues and bugs like what this patch is fixing), macros (which IMO are harder to read than goto), or a separate function for cleanup (annoying to maintain, function call overhead). http://onlinehut.org/2011/10/goto-is-not-evil-okay/
On 06/11/2014 09:44 PM, Sean Farley wrote: > Kevin Bullock writes: > >> On Jun 11, 2014, at 5:43 PM, danek.duvall@oracle.com wrote: >> >>> # HG changeset patch >>> # User Danek Duvall <danek.duvall@oracle.com> >>> # Date 1402525864 25200 >>> # Wed Jun 11 15:31:04 2014 -0700 >>> # Branch stable >>> # Node ID b091b262aa6d25e7e4663672daf2b6d8f127ab48 >>> # Parent b35f8c487e396487e89f98e92da57ac5eb9833af >>> parsers.c: fix a couple of memory leaks >>> >>> diff --git a/mercurial/parsers.c b/mercurial/parsers.c >>> --- a/mercurial/parsers.c >>> +++ b/mercurial/parsers.c >>> @@ -1403,8 +1403,12 @@ static PyObject *find_deepest(indexObjec >>> final |= i; >>> j -= 1; >>> } >>> - if (final == 0) >>> + if (final == 0) { >>> + free(depth); >>> + free(seen); >>> + free(interesting); >>> return PyList_New(0); >>> + } >> Good catch, but this is now the 3rd place we've got those same `free()`s in this function. Would something like this work instead: >> >> diff --git a/mercurial/parsers.c b/mercurial/parsers.c >> --- a/mercurial/parsers.c >> +++ b/mercurial/parsers.c >> @@ -1403,8 +1403,10 @@ static PyObject *find_deepest(indexObjec >> final |= i; >> j -= 1; >> } >> - if (final == 0) >> - return PyList_New(0); >> + if (final == 0) { >> + keys = PyList_New(0); >> + goto cleanup; > http://xkcd.com/292/ As Greg said, goto is absolutely the right choice for error handling in C. I generally don't consider forward gotos to be a problem. It is *backward* gotos that get really hairy.
Siddharth Agarwal writes: > On 06/11/2014 09:44 PM, Sean Farley wrote: >> Kevin Bullock writes: >> >>> On Jun 11, 2014, at 5:43 PM, danek.duvall@oracle.com wrote: >>> >>>> # HG changeset patch >>>> # User Danek Duvall <danek.duvall@oracle.com> >>>> # Date 1402525864 25200 >>>> # Wed Jun 11 15:31:04 2014 -0700 >>>> # Branch stable >>>> # Node ID b091b262aa6d25e7e4663672daf2b6d8f127ab48 >>>> # Parent b35f8c487e396487e89f98e92da57ac5eb9833af >>>> parsers.c: fix a couple of memory leaks >>>> >>>> diff --git a/mercurial/parsers.c b/mercurial/parsers.c >>>> --- a/mercurial/parsers.c >>>> +++ b/mercurial/parsers.c >>>> @@ -1403,8 +1403,12 @@ static PyObject *find_deepest(indexObjec >>>> final |= i; >>>> j -= 1; >>>> } >>>> - if (final == 0) >>>> + if (final == 0) { >>>> + free(depth); >>>> + free(seen); >>>> + free(interesting); >>>> return PyList_New(0); >>>> + } >>> Good catch, but this is now the 3rd place we've got those same `free()`s in this function. Would something like this work instead: >>> >>> diff --git a/mercurial/parsers.c b/mercurial/parsers.c >>> --- a/mercurial/parsers.c >>> +++ b/mercurial/parsers.c >>> @@ -1403,8 +1403,10 @@ static PyObject *find_deepest(indexObjec >>> final |= i; >>> j -= 1; >>> } >>> - if (final == 0) >>> - return PyList_New(0); >>> + if (final == 0) { >>> + keys = PyList_New(0); >>> + goto cleanup; >> http://xkcd.com/292/ > > As Greg said, goto is absolutely the right choice for error handling in > C. I generally don't consider forward gotos to be a problem. It is > *backward* gotos that get really hairy. Sorry. I actually agree with both you and Greg. I just couldn't help make the reference to xkcd :-)
Patch
diff --git a/mercurial/parsers.c b/mercurial/parsers.c --- a/mercurial/parsers.c +++ b/mercurial/parsers.c @@ -1403,8 +1403,10 @@ static PyObject *find_deepest(indexObjec final |= i; j -= 1; } - if (final == 0) - return PyList_New(0); + if (final == 0) { + keys = PyList_New(0); + goto cleanup; + } dict = PyDict_New(); if (dict == NULL) @@ -1427,20 +1429,17 @@ static PyObject *find_deepest(indexObjec } keys = PyDict_Keys(dict); + goto cleanup; +bail: + keys = NULL; +cleanup: free(depth); free(seen); free(interesting); Py_DECREF(dict); return keys; -bail: - free(depth); - free(seen); - free(interesting); - Py_XDECREF(dict); - - return NULL; } /*