Patchwork [2,of,5] lazymanifest: simplify cleanup at end of iteration

login
register
mail settings
Submitter Martin von Zweigbergk
Date March 11, 2015, 11:14 p.m.
Message ID <7daaf234ddc08ee8dd17.1426115664@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/8012/
State Changes Requested
Headers show

Comments

Martin von Zweigbergk - March 11, 2015, 11:14 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1426105596 25200
#      Wed Mar 11 13:26:36 2015 -0700
# Node ID 7daaf234ddc08ee8dd171a6c2d3347e0caf1632e
# Parent  0709db5b7ee275833b41b89feab59625dca28c7a
lazymanifest: simplify cleanup at end of iteration

When the iterator has been exhausted, there is nothing to clean up, so
return early and simplify the remaining code.
Matt Mackall - March 12, 2015, 10:49 p.m.
On Wed, 2015-03-11 at 16:14 -0700, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1426105596 25200
> #      Wed Mar 11 13:26:36 2015 -0700
> # Node ID 7daaf234ddc08ee8dd171a6c2d3347e0caf1632e
> # Parent  0709db5b7ee275833b41b89feab59625dca28c7a
> lazymanifest: simplify cleanup at end of iteration
> 
> When the iterator has been exhausted, there is nothing to clean up, so
> return early and simplify the remaining code.

I actually prefer the goto pattern. It makes maintaining code with
multiple exit points and (especially nested) resource unrolling much
more manageable. And since there's always a risk we'll add new exit
points here, I'd rather not take a step back here.

See here for a better example of the pattern:

http://www.selenic.com/hg/file/7cf9a9e0cf89/mercurial/osutil.c#l185

And see here for a discussion (chapter 7):

https://www.kernel.org/doc/Documentation/CodingStyle
Martin von Zweigbergk - March 12, 2015, 11 p.m.
Fair enough. I'll drop the patch and send a V2 of the remaining 3. Thanks.

On Thu, Mar 12, 2015 at 3:49 PM Matt Mackall <mpm@selenic.com> wrote:

> On Wed, 2015-03-11 at 16:14 -0700, Martin von Zweigbergk wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1426105596 25200
> > #      Wed Mar 11 13:26:36 2015 -0700
> > # Node ID 7daaf234ddc08ee8dd171a6c2d3347e0caf1632e
> > # Parent  0709db5b7ee275833b41b89feab59625dca28c7a
> > lazymanifest: simplify cleanup at end of iteration
> >
> > When the iterator has been exhausted, there is nothing to clean up, so
> > return early and simplify the remaining code.
>
> I actually prefer the goto pattern. It makes maintaining code with
> multiple exit points and (especially nested) resource unrolling much
> more manageable. And since there's always a risk we'll add new exit
> points here, I'd rather not take a step back here.
>
> See here for a better example of the pattern:
>
> http://www.selenic.com/hg/file/7cf9a9e0cf89/mercurial/osutil.c#l185
>
> And see here for a discussion (chapter 7):
>
> https://www.kernel.org/doc/Documentation/CodingStyle
>
> --
> Mathematics is the supreme nostalgia of our time.
>
>
>

Patch

diff -r 0709db5b7ee2 -r 7daaf234ddc0 mercurial/manifest.c
--- a/mercurial/manifest.c	Wed Mar 11 08:28:56 2015 -0700
+++ b/mercurial/manifest.c	Wed Mar 11 13:26:36 2015 -0700
@@ -227,12 +227,12 @@ 
 	size_t pl;
 	line *l;
 	Py_ssize_t consumed;
-	PyObject *ret = NULL, *path = NULL, *hash = NULL, *flags = NULL;
+	PyObject *path = NULL, *hash = NULL, *flags = NULL;
 	lmIter *self = (lmIter *)o;
 	do {
 		self->pos++;
 		if (self->pos >= self->m->numlines) {
-			goto bail;
+			return NULL;
 		}
 		/* skip over deleted manifest entries */
 	} while (self->m->lines[self->pos].deleted);
@@ -243,15 +243,13 @@ 
 	consumed = pl + 41;
 	flags = PyString_FromStringAndSize(l->start + consumed,
 									   l->len - consumed - 1);
-	if (!flags) {
-		goto bail;
+	if (flags) {
+		return PyTuple_Pack(3, path, hash, flags);
+	} else {
+		Py_XDECREF(path);
+		Py_XDECREF(hash);
+		return NULL;
 	}
-	ret = PyTuple_Pack(3, path, hash, flags);
- bail:
-	Py_XDECREF(path);
-	Py_XDECREF(hash);
-	Py_XDECREF(flags);
-	return ret;
 }
 
 static PyTypeObject lazymanifestIterator = {