Ramkumar Ramachandra wrote:
> 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:
> +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).
> +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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html