Ramkumar Ramachandra wrote:

quoted text
> void pre_commit(void);
> 
> 	Write the pool to file.

Except as a proof of concept, this is the wrong API to have.  The problem
is that the caller cannot choose the filename, so it ends up being a .bin
file in the current directory, wherever that is.

The log message leaves out a subtlety: this also increases the
‘committed’ value, and bookkeeping for that might be useful to some
callers.

In other words:

quoted text
> +static MAYBE_UNUSED void pre##_init(void) \
> +{ \
> +	struct stat st; \
> +	pre##_pool.file = fopen(#pre ".bin", "a+"); \
> +	rewind(pre##_pool.file); \
> +	fstat(fileno(pre##_pool.file), &st); \
> +	pre##_pool.size = st.st_size / sizeof(obj_t); \
> +	pre##_pool.committed = pre##_pool.size; \
> +	pre##_pool.capacity = pre##_pool.size * 2; \
> +	if (pre##_pool.capacity < initial_capacity) \
> +		pre##_pool.capacity = initial_capacity; \
> +	pre##_pool.base = malloc(pre##_pool.capacity * sizeof(obj_t)); \
> +	fread(pre##_pool.base, sizeof(obj_t), pre##_pool.size, pre##_pool.file); \
> +} \

If you just want something working, I’d suggest stubbing this out:

 static MAYBE_UNUSED void pre##_init(void) \
 { \
 } \

It even almost makes sense as API: the _init function does all
initialization tasks required, which is to say, none.  (The {0, ...}
initializer already has taken care of setting all fields to 0).

quoted text
> +static MAYBE_UNUSED void pre##_commit(void) \
> +{ \
> +	pre##_pool.committed += fwrite(pre##_pool.base + pre##_pool.committed, \
> +		sizeof(obj_t), pre##_pool.size - pre##_pool.committed, \
> +		pre##_pool.file); \
> +} \

This can be simplified

 static MAYBE_UNUSED void pre##_commit(void) \
 { \
	pre##_pool.committed = pre##_pool.size; \
 } \

In other words, maybe something like this on top?  This includes the
vestigal _init() function which really should not be there (it is
confusing that some callers use it and others don’t).  I did not
spend much time on it because in the end I suspect we might throw
obj_pool away anyway.

---
diff --git a/Makefile b/Makefile
index fc31ee0..386a586 100644
--- a/Makefile
+++ b/Makefile
@@ -409,6 +409,7 @@ TEST_PROGRAMS_NEED_X += test-delta
 TEST_PROGRAMS_NEED_X += test-dump-cache-tree
 TEST_PROGRAMS_NEED_X += test-genrandom
 TEST_PROGRAMS_NEED_X += test-match-trees
+TEST_PROGRAMS_NEED_X += test-obj-pool
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
 TEST_PROGRAMS_NEED_X += test-run-command
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 680d7d6..262f304 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -12,4 +12,10 @@ test_expect_success 'character classes (isspace, isalpha etc.)' '
 	test-ctype
 '
 
+test_expect_success 'allocator for svn importer' '
+	printf "%s\n" 0 0 0 0 0 2 2 2 2 2 -1 >expected &&
+	test-obj-pool >actual &&
+	test_cmp expected actual
+'
+
 test_done
diff --git a/test-obj-pool.c b/test-obj-pool.c
new file mode 100644
index 0000000..1300049
--- /dev/null
+++ b/test-obj-pool.c
@@ -0,0 +1,55 @@
+/*
+ * test-obj-pool.c: code to exercise the svn importer's object pool
+ */
+
+#include "vcs-svn/obj_pool.h"
+
+static const char usage_str[] =
+	"test-obj-pool";
+
+obj_pool_gen(int, int, 2)
+obj_pool_gen(other, int, 4096)
+
+int main(int argc, char *argv[])
+{
+	int *p;
+	int i;
+
+	if (argc != 1) {
+		fprintf(stderr, "Usage: %s\n", usage_str);
+		return 1;
+	}
+
+	int_init();
+
+	p = int_pointer(int_alloc(10));
+	for (i = 0; i < 10; i++)
+		p[i] = 0;
+
+	*other_pointer(other_alloc(1)) = -1;
+	other_commit();
+
+	p = other_pointer(other_alloc(10));
+	for (i = 0; i < 10; i++)
+		p[i] = 1;
+
+	int_free(5);
+
+	p = int_pointer(int_alloc(10));
+	for (i = 0; i < 10; i++)
+		p[i] = 2;
+
+	int_free(5);
+	int_commit();
+
+	for (i = 0; i < int_pool.committed; i++)
+		printf("%d\n", *int_pointer(i));
+
+	for (i = 0; i < other_pool.committed; i++)
+		printf("%d\n", *other_pointer(i));
+
+	int_reset();
+	int_reset();
+	other_reset();
+	return 0;
+}
diff --git a/vcs-svn/obj_pool.h b/vcs-svn/obj_pool.h
index f60c872..90eda15 100644
--- a/vcs-svn/obj_pool.h
+++ b/vcs-svn/obj_pool.h
@@ -16,21 +16,9 @@ static struct { \
 	uint32_t size; \
 	uint32_t capacity; \
 	obj_t *base; \
-	FILE *file; \
-} pre##_pool = { 0, 0, 0, NULL, NULL}; \
+} pre##_pool = {0, 0, 0, NULL}; \
 static MAYBE_UNUSED void pre##_init(void) \
 { \
-	struct stat st; \
-	pre##_pool.file = fopen(#pre ".bin", "a+"); \
-	rewind(pre##_pool.file); \
-	fstat(fileno(pre##_pool.file), &st); \
-	pre##_pool.size = st.st_size / sizeof(obj_t); \
-	pre##_pool.committed = pre##_pool.size; \
-	pre##_pool.capacity = pre##_pool.size * 2; \
-	if (pre##_pool.capacity < initial_capacity) \
-		pre##_pool.capacity = initial_capacity; \
-	pre##_pool.base = malloc(pre##_pool.capacity * sizeof(obj_t)); \
-	fread(pre##_pool.base, sizeof(obj_t), pre##_pool.size, pre##_pool.file); \
 } \
 static MAYBE_UNUSED uint32_t pre##_alloc(uint32_t count) \
 { \
@@ -62,19 +50,15 @@ static MAYBE_UNUSED obj_t *pre##_pointer(uint32_t offset) \
 } \
 static MAYBE_UNUSED void pre##_commit(void) \
 { \
-	pre##_pool.committed += fwrite(pre##_pool.base + pre##_pool.committed, \
-		sizeof(obj_t), pre##_pool.size - pre##_pool.committed, \
-		pre##_pool.file); \
+	pre##_pool.committed = pre##_pool.size; \
 } \
 static MAYBE_UNUSED void pre##_reset(void) \
 { \
 	free(pre##_pool.base); \
-	if (pre##_pool.file) \
-		fclose(pre##_pool.file); \
 	pre##_pool.base = NULL; \
 	pre##_pool.size = 0; \
 	pre##_pool.capacity = 0; \
-	pre##_pool.file = NULL; \
+	pre##_pool.committed = 0; \
 }
 
 #endif
-- 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html