Hi Jonathan,
Jonathan Nieder writes:
> 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.
Oh, right. I remember that you asked to turn off persistence for this
merge. We can include persistence it in a later series.
Junio: Could you squash this diff into the commit?
diff --git a/vcs-svn/obj_pool.h b/vcs-svn/obj_pool.h
index f60c872..7a256d4 100644
--- a/vcs-svn/obj_pool.h
+++ b/vcs-svn/obj_pool.h
@@ -20,17 +20,6 @@ static struct { \
} pre##_pool = { 0, 0, 0, NULL, 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,9 +51,7 @@ 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) \
{ \
--
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