are these msec<->timeval functions correct?

Question

I have a bug in this program, and I keep coming back to these two functions, but they look right to me. Anything wrong here?

long visual_time_get_msec(VisTime *time_)
{
    visual_log_return_val_if_fail(time_ != NULL, 0);

    return time_->tv_sec * 1000 + time_->tv_usec / 1000;
}


int visual_time_set_from_msec(VisTime *time_, long msec)
{
    visual_log_return_val_if_fail(time_ != NULL, -VISUAL_ERROR_TIME_NULL);


    long sec = msec / 1000;
    long usec = 0;

    visual_time_set(time_, sec, usec);

    return VISUAL_OK;
}

Answer 1

visual_time_set_from_msec doesnt look right...

if someone calls visual_time_set_from_msec(time, 999), then your struct will be set to zero, rather the 999,000us.

What you should do is:

// Calculate number of seconds
long sec = msec / 1000; 
// Calculate remainding microseconds after number of seconds is taken in to account
long usec = (msec - 1000*sec) * 1000;

it really depends on your inputs, but thats my 2 cents :-)

Answer 2

Your first function is rounding down, so that 1.000999 seconds is rounded to 1000ms, rather than 1001ms. To fix that (make it round to nearest millisecond), you could do this:

long visual_time_get_msec(VisTime *time_)
{
    visual_log_return_val_if_fail(time_ != NULL, 0);

    return time_->tv_sec * 1000 + (time_->tv_usec + 500) / 1000;
}

Fuzz has already pointed out the truncation in your second example - the only thing I would add is that you can simplify it a little using the modulo operator:

long sec = msec / 1000;
long usec = (msec % 1000) * 1000;

(The above all assume that you're not dealing with negative timevals - if you are, it gets more complicated).