[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