Re: [PATCH 5/7] Add API for string-specific memory pool

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 29, 2010, 11:38 UTC
Message-ID: <20100529113800.GA7925@progeny.tock>
In-Reply-To: <1274650832-7411-6-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

> string_pool uses the macros in the memory pool library to create a
> memory pool for strings and expose an API for handling the
> strings.

This interns strings so they can be compared by address. Interesting. The use of offsets instead of pointers here mean it is possible to back the pool by a file, if I understand correctly.

> Taken directly from David Michael Barr's svn-dump-fast-export
> repository.

Missing From: and sign-off.

[...]

Show 5 quoted lines

> +static int node_indentity_cmp(node_t *a, node_t *b)
> +{
> +    int r = node_value_cmp(a, b);
> +    return r ? r : (((uintptr_t) a) > ((uintptr_t) b))
> +        - (((uintptr_t) a) < ((uintptr_t) b));

nitpick: could use fewer parentheses.

	return r ? r : (((uintptr_t) a > (uintptr_t) b) - ...

Are the casts to uintptr_t necessary? C99 says, under "Relational operators", paragraph 5:

  When two pointers are compared, the result depends on the relative
  locations in the address space of the objects pointed to. If two
  pointers to object or incomplete types both point to the same
  object, or both point one past the last element of the same array
  object, they compare equal. If the objects pointed to are members of
  the same aggregate object,
  [etc]

which seems to suggest that no, simple expressions like (a > b) should be okay, but then it finishes with

  In all other cases, the behavior is undefined.

so I guess it is safer to keep the casts.

Show 8 quoted lines

> diff --git a/vcs-svn/string_pool.h b/vcs-svn/string_pool.h
> new file mode 100644
> index 0000000..fb9e6b8
> --- /dev/null
> +++ b/vcs-svn/string_pool.h
> @@ -0,0 +1,11 @@
> +#ifndef STRING_POOL_H_
> +#define	STRING_POOL_H_

style nitpick: should use space instead of tab

> +
> +#include <stdint.h>
> +#include <stdio.h>

Should use "../git-compat-util.h" for portability: unfortunately, some platforms git supports still don’t have stdint.h iirc.

Except as noted above,

  Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for the pleasant read. Jonathan