8
\$\begingroup\$

I've noticed some discrepancies in timings in our system, and I'm trying to narrow down what could be causing them.

I'm reviewing out time abstraction, and as far as I can determine it's fine.

Am I missing anything, and is it portable (besides being constrained to POSIX)?

typedef struct timespec Time;
static inline Time getTime() {
    static struct timespec time;
    clock_gettime(CLOCK_MONOTONIC, &time);
    return time;
}
static inline __constant Time timeAdd(Time oldTime, Time time) {
    if (time.tv_nsec + oldTime.tv_nsec >= 1E9)
        return (Time){
            tv_sec: time.tv_sec + oldTime.tv_sec + 1,
            tv_nsec: time.tv_nsec + oldTime.tv_nsec - 1E9
        };
    else
        return (Time){
            tv_sec: time.tv_sec + oldTime.tv_sec,
            tv_nsec: time.tv_nsec + oldTime.tv_nsec
        };
}
static inline __constant Time timeDiff(Time oldTime, Time time) {
    if (time.tv_nsec < oldTime.tv_nsec)
        return (Time){
            tv_sec: time.tv_sec - 1 - oldTime.tv_sec,
            tv_nsec: 1E9 + time.tv_nsec - oldTime.tv_nsec
        };
    else
        return (Time){
            tv_sec: time.tv_sec - oldTime.tv_sec,
            tv_nsec: time.tv_nsec - oldTime.tv_nsec
        };
}
static __always_inline __constant double timeSeconds(Time time) {
    return time.tv_sec + time.tv_nsec/1E9;
}
static inline double timeSince(Time oldTime) {
    struct timespec time;
    clock_gettime(CLOCK_MONOTONIC, &time);
    return timeSeconds(timeDiff(oldTime, time));
}
\$\endgroup\$
10
\$\begingroup\$

I can see nothing wrong, but I think it is not optimal. My compiler warns me about floating point conversions of those '1E9' constants. If I rewrite your timeAdd as follows, the code is easier to read and the generated code is significantly smaller:

#define BILLION 1000000000
Time timeAdd(Time t1, Time t2)
{
    long sec = t2.tv_sec + t1.tv_sec;
    long nsec = t2.tv_nsec + t1.tv_nsec;
    if (nsec >= BILLION) {
        nsec -= BILLION;
        sec++;
    }
    return (Time){ .tv_sec = sec, .tv_nsec = nsec };
}

The same goes for the other functions, I think.

\$\endgroup\$
  • 2
    \$\begingroup\$ If you're using integer BILLION then the timeSeconds expression should include a cast to double, i.e. time.tv_sec + (double)time.tv_nsec/BILLION; \$\endgroup\$ – ChrisW Jan 27 '14 at 20:58
  • \$\begingroup\$ I haven't seen you answer any questions in a while. I sure hope we haven't seen the last of William Morris. \$\endgroup\$ – syb0rg Feb 27 '14 at 1:20
  • 1
    \$\begingroup\$ @syb0rg not intentionally, but I only really review C and there are some great reviewers (yourself included) who seem always to beat me to it recently :-) \$\endgroup\$ – William Morris Feb 27 '14 at 13:37
  • \$\begingroup\$ @WilliamMorris That's usually because I hang out in our chat room, and there is a constantly updating feed of new questions so I can see when they are posted. :P \$\endgroup\$ – syb0rg Feb 27 '14 at 15:17
3
\$\begingroup\$

Just be careful with your double timeSeconds(Time time) function, since a double does not have enough bits to fully express a large number of seconds and a small number of fractional seconds. For example, using a timestamp in the year of 2014, resolution for fractions of a second starts breaking down somewhere around 10us.

\$\endgroup\$
1
\$\begingroup\$

You may also want an implementation for timeCmp in order to complete your library:

// Return 1 if a > b, -1 if a < b, 0 if a == b
static inline __constant int timeCmp(Time a, Time b) {
    if (a.tv_sec != b.tv_sec) {
        if (a.tv_sec > b.tv_sec)
            return 1;
        else 
            return -1;
    } else {
        if (a.tv_nsec > b.tv_nsec)
            return 1;
        else if (a.tv_nsec < b.tv_nsec)
            return -1;
        else
            return 0;
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.