dovecot - Apr 2012 - array code issue ?

Hi,

I just took a look into the dovecot 2.1 sources and just saw a possible issue 
in array.h.

This code snippet as an example:
#static inline void *
#array_get_modifiable_i(struct array *array, unsigned int *count_r)
#{
#	*count_r = array->buffer->used / array->element_size;
#	return buffer_get_modifiable_data(array->buffer, NULL);
#}

array->buffer->used and array->element_size are of type
'size_t' which is
64bit on amd64 and others while 'count_r' is a 32bit value. At least, I
see
ugly warnings with -Wconversion (which I personally like to use).

I know, it is unlikely that 'array->buffer->used /
array->element_size'
exceeds 32bit range. But then, dovecot's source is so well written, that the
above code seems to disturb dovecot's code aesthetics.

And who knows... in a few years (when we have THz and TBytes on our desktops) 
emails (and array sizes) might exceed everything that we think of today.

Tim

On 20.4.2012, at 17.27, Tim Ruehsen wrote:
> I just took a look into the dovecot 2.1 sources and just saw a possible
issue
> in array.h.
> 
> This code snippet as an example:
> #static inline void *
> #array_get_modifiable_i(struct array *array, unsigned int *count_r)
> #{
> #	*count_r = array->buffer->used / array->element_size;
> #	return buffer_get_modifiable_data(array->buffer, NULL);
> #}
> 
> array->buffer->used and array->element_size are of type
'size_t' which is
> 64bit on amd64 and others while 'count_r' is a 32bit value. At
least, I see
> ugly warnings with -Wconversion (which I personally like to use).
I've been planning on trying out some of clang's warning flags. Last
time I used -Wconversion with gcc it was giving way too many warnings to be
usable, but clang's -Wconversion looked better when I quickly looked at it.
> I know, it is unlikely that 'array->buffer->used /
array->element_size'
> exceeds 32bit range. But then, dovecot's source is so well written,
that the
> above code seems to disturb dovecot's code aesthetics.
:) Yeah, I intentionally decided to use unsigned int here. It's a bit of
wasteful and ugly to use size_t everywhere.. I guess the code could be made
something like:

size_t count = array->buffer->used / array->element_size;
I_assert(count < UINT_MAX);
*count_r = (unsigned int)count;

Or something like that. Although these array functions are sometimes in
performance critical paths, so adding extra code isn't very good either.
Perhaps a simple cast to make the warning go away.. Probably the element_size
could also be changed to be unsigned int.
> And who knows... in a few years (when we have THz and TBytes on our
desktops)
> emails (and array sizes) might exceed everything that we think of today.
The email sizes yes, but probably not the number of emails in a mailbox.