On Fri, Jun 15, 2018 at 10:05:42AM -0700, Junio C Hamano wrote:
> > - memcpy(self->buffer, ptr, self->buffer_size * sizeof(eword_t));
> > - ptr += self->buffer_size * sizeof(eword_t);
>
>
> > + data_len = st_mult(self->buffer_size, sizeof(eword_t));
>
> This is a faithful conversion from the original, but I somehow would
> have appreciated if the latter were not sizeof(eword_t) but rather
> sizeof(self->buffer_size[0]), especially as I wondered ...
I actually thought about going the _other_ way. The sizeof(eword_t) is
not something we can change, but is actually decided by the on-disk
format. So I wondered if this should be much more clearly "hey, this is
8 bytes". Possibly with an assert(sizeof(*self->buffer_size) == 8).
And yes, I think having the on-disk format specify the size in 8-byte
double words is vaguely crazy. Blame JGit. ;) Or maybe even blame the
original EWAH authors, this may have originated even earlier (I didn't
dig).
> > + if (len < data_len)
> > + return error("corrupt ewah bitmap: eof in data "
> > + "(%"PRIuMAX" bytes short)",
> > + (uintmax_t)(data_len - len));
> > + memcpy(self->buffer, ptr, data_len);
> > + ptr += data_len;
> > + len -= data_len;
> >
> > for (i = 0; i < self->buffer_size; ++i)
> > self->buffer[i] = ntohll(self->buffer[i]);
>
> ... what individual datum one iteration of this loop is copying, and
> then realized "buffer_size" is a misleading field name (anything that
> claims to be size and not measuring in bytes is misleading to me ;-).
Yeah, it confused me at first, too. I don't mind changing these kinds of
cosmetics, but I'd like to do it in a separate patch from this fix.
> > - return (3 * 4) + (self->buffer_size * 8);
> > + return ptr - (const uint8_t *)map;
>
> Much nicer; I needed to wonder what these 12 and 8 in the original are.
Me too. ;)
> > int ewah_deserialize(struct ewah_bitmap *self, int fd);
> > -int ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len);
> > +ssize_t ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len);
>
> I double checked all the callers and made sure that they are already
> prepared to react sensibly to error returns, which is good.
Yep, modulo the int/ssize_t thing from the fourth patch.
-Peff
