[PATCH 1 of 4 lazy-manifest] manifest.c: new extension code to lazily parse manifests

Matt Mackall mpm at selenic.com
Thu Jan 8 15:11:59 CST 2015


On Thu, 2015-01-08 at 15:34 -0500, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie at google.com>
> # Date 1420748959 18000
> #      Thu Jan 08 15:29:19 2015 -0500
> # Node ID 788bb2dacad048f161dfed583d3a95f708e91d1e
> # Parent  7ad155e13f0f51df8e986a0ec4e58ac9a0ccedbb
> manifest.c: new extension code to lazily parse manifests
> 
> This lets us iterate manifests in order, but do a _lot_ less work in
> the common case when we only care about a few manifest entries.
> 
> Many thanks to Mike Edgar for reviewing this in advance of it going
> out to the list, which caught many things I missed.

I'm going to do a quick coding style pass first.

> +/* yay c89 */
> +#define true 1
> +#define false 0
> +typedef unsigned char bool;

If we must have this, it probably belongs in util.h.

> +const int kDefaultLinesSize = 100000;

Camel case. k??

> +static int realloc_if_needed(lazymanifest * self)

Consider the actual semantics vs the obvious reading of the following:

foo *a, b, c; /* one pointer, two objects */
foo* d, e, f; /* three pointers? */
foo * g, h, i; /* multiplication!? */

..and you will discover where the * should always go.

> +		if (point == NULL) {

if (!point) is preferred

> +			line *l = self->lines + ((self->numlines)++);

You're doing something weird with the parens here. C definitely can't
increment "numlines" nor "self->lines + self->numlines".

> +			if (*point == '\0') {

if (*point) { } else { break; } /* inverted */

> +	self->lines = (line *)malloc(kDefaultLinesSize * sizeof(line));

Everyone loves to stick these casts here but they're bogus. malloc
return "void *" which is automatically promoted to the right type.

> +/* defined in parsers.c */
> +PyObject *unhexlify(const char *str, int len);

Header file?

> +	if (hit == NULL || hit->deleted == true) {

if (!hit || hit->deleted)

> +	if (!PyObject_IsInstance(value, (PyObject *) & PyTuple_Type)

Same story with & as with *. This looks like a bitwise operation to my
eyes. Casts should always be (foo *)&var.

I bet there's a tuple test somewhere.

Could probably stand to have some function level comments on some of
these:

> +static int compact(lazymanifest *self) {

-- 
Mathematics is the supreme nostalgia of our time.




More information about the Mercurial-devel mailing list