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

[Jonathan Nieder <jrnieder@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.

[...]
> +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.

> 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