Parsing strings indicating duration in seconds (e.g. "60", "60s", "1m", etc.)

Question

This is a simple function that accepts a null-terminated string that represents a non-negative number of seconds. It can optionally end in the suffix "s" (seconds, default), "m" (minutes), "h" (hours), or "d" (days). The number is read by strtod and so it can accept floating point notation. For example all of these return 216000: "2.5d", "60h", "3600m", "216000s", "216000". I understand the limitations of floating point types and it's not a problem if not every time_t value can be precisely represented, nor is it a problem that the returne number of seconds will be rounded down to the nearest integer.

The function returns -1 on invalid input, TIME_MAX on input that would have exceeded the capacity of time_t to represent, and an integer number of seconds otherwise. Performance is not particularly important, but avoiding edge cases is.

The function will only run on POSIX systems so I can make assumptions, such as assuming time_t is a signed integer (POSIX guarantees that even though the C standard only states that it is an "arithmetic type").

The contents of parse_time.c:

#include <stdlib.h>
#include <stdint.h>
#include <string.h>
#include <limits.h>
#include <time.h>
#include <math.h>
#include <errno.h>
#include <assert.h>

#include "parse_time.h"

time_t parse_time(const char *str)
{
    char *endp;
    double time;

    assert(str);

    /* limit to decimal floating point without leading whitespace, a sign, or an exponent */
    if (isspace(*str) || strpbrk(str, "eExX+-") != NULL)
        return -1;

    errno = 0;
    time = strtod(str, &endp);

    /* verify that the time is non-negative and does not overflow */
    if (errno == ERANGE || endp == str || time < 0)
        return -1;

    /* verify that the float has no suffix or has only a one character suffix */
    if (*endp != '\0' && *(endp + 1) != '\0')
        return -1;

    /* handle any suffix */
    switch (*endp) {
    case '\0':
    case 's':
        break;
    case 'm':
        time *= SECS_PER_MIN;
        break;
    case 'h':
        time *= SECS_PER_HOUR;
        break;
    case 'd':
        time *= SECS_PER_DAY;
        break;
    default:
        return -1;
    }

    if (isnan(time))
        return -1;

    /* cap at the maximum value time_t can represent */
    return (time >= TIME_MAX) ? TIME_MAX : (time_t)time;
}

The contents of parse_time.h:

#include <assert.h>
#include <time.h>

/* get maximum value that time_t can represent */
#if __TIMESIZE == 64
#define TIME_MAX INT64_MAX
#else
#define TIME_MAX INT32_MAX
#endif
static_assert(TIME_MAX == (time_t)TIME_MAX);

#define SECS_PER_MIN    60
#define SECS_PER_HOUR   3600
#define SECS_PER_DAY    86400

time_t parse_time(const char *str);

Answer 1

time as a double?

Certainly confusing to have double time as time is a standard function name. More clear to use a different name.

Capped value?

When (time >= TIME_MAX) is true, code returns a capped value. This is inconsistent with prior code that does if (errno == ERANGE || ...) return -1;. Prior code errors with crazy large conversions, yet caps with lessor out of time_t range values.

Consistent functionality would return -1 when time is outside time_t range.

Weakness in range test

On 64-bit machines time >= TIME_MAX attempts conversion of INT64_MAX to a double. This is highly unlikely to be exact. Instead of the value INT64_MAX, the compare will happen with (double) INT64_MAX, a value near INT64_MAX.

Since OP is truncating the fraction, a better exact compare:

#define TIME_MAX_PLUS_1 ((TIME_MAX/2 + 1)*2.0)
// return (time >= TIME_MAX) ? TIME_MAX : (time_t)time;
return (time >= TIME_MAX_PLUS_1) ? TIME_MAX : (time_t)time;

Truncated Fractions?

(time_t)time; truncates any fraction. I'd expect a rounded conversion rather than a truncated one. Yes this is a design change, but it is the functionality that I suspect users would prefer.

// (time_t)time
llround(time)

Name space pollution

Code defines TIME_MAX, SECS_PER_MIN, SECS_PER_HOUR, SECS_PER_DAY in a header called parse_time.h.

Based on name, I would not expect to find them in that header. Maybe precede each with parse_time_ with the matching case.

SECS_PER_MIN, SECS_PER_HOUR, SECS_PER_DAY can readily collides with other code. Again, precede each with parse_time_ or the like. Even better, just move these 3 defines into parse_time.c.

Improve parse_time.h

.c files should be able to include parse_time.h multiple times without trouble. parse_time.h should have code guards.

Including parse_time.h should not rely on the .c file first including other header files. If parse_time.h needs standard header files, (which it does), it should include them itself.

For parse_time.c only, consider re-ordering with its paired header file included twice and first in the list to test the above properties.

#include "parse_time.h"
#include "parse_time.h"
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
#include <limits.h>
#include <time.h>
#include <math.h>
#include <errno.h>
#include <assert.h>

Minor note: Standard header files should be able to be included in any order. I A-Z sort them to reduce maintenance issues and ease review.

Design: more strto...() like

Rather than time_t parse_time(const char *str), consider time_t parse_time(const char *str, char **endptr) to mimic strtod(), strtol(), ... and report the end of the conversion.

Future Design Growth: Allow compound times

Example: Allow "11h59m59s".

Design: No negatives

Not allowing a leading sign unnecessarily reduces the usefulness on this function.

Design: No exponent

I feel less strong about this:
Not allowing an exponent unnecessarily reduces the usefulness on this function.

Design: Maybe allow case-insensitive suffixes?

(or maybe not as future versions may use M for month.)

Design: Maybe add w for weeks?

Minor: redundant test

if (errno == ERANGE || endp == str || time < 0) checks time < 0, yet prior code tested for the presence of a '-'.

Minor: late test

Consider making if (isnan(time)) return -1; part of the if (*endp != '\0' && *(endp + 1) != '\0') return -1;.

Minor: string

As the C library defines a string as ending with a null character, "accepts a null-terminated string" could be replaced with "accepts a string" or even technically better as "accepts a pointer to a string", (str is a pointer with const char *str) unless you worry about folks thinking of string as something from another language.

A string is a contiguous sequence of characters terminated by and including the first null character.
C2X § 7.1.1 1

---

Deeper ideas

time_t is a calendar time, like Aug 12, 2024 13:23:45. This parse_time() seems to be more about a duration time, like the time in seconds of 2.5 days. Units like seconds, minutes, hours, days, and even weeks can work for both times. Once we start using months, years, we have trouble with duration as the length of a month (28-31 days) and year (365-366 days) varies a bit.

Years ago, I wrote a similar function that allowed strings like "-4D11h59m59s" and used a leading sign to indicate duration and a sign-less string to indicate a calendar time. Calendar times allowed a month and year.

---

Reference

Use ISO-8601 to guide parsing of a string into time_t.

Answer 2

Although the description says this will only be built for POSIX targets, it's a good idea to test that condition, as code tends to get re-used unexpectedly:

#ifndef _POSIX_C_SOURCE
#error POSIX environment required
#endif

---

We have an error here:

   if (isspace(*str)

*str is a plain char, but isspace() accepts a positive integer, so we must cast to unsigned char before integer promotion happens.

Consider allowing leading whitespace, like C's own numeric conversions.

---

Instead of parsing a double with restrictions, it may be simpler to read 1 or 2 integers separated by . and treat the result as a rational number. That should give better precision when the value is large.

---

The SECS_PER_* macros don't need to be in the public header. And I'm sure they don't need to be macros.

---

We can simplify the suffix-checking code:

    /* handle optional unit suffix */
    switch (*endp) {
    case 'w':
        time *= 7;
        [[fallthrough]];
    case 'd':
        time *= 24;
        [[fallthrough]];
    case 'h':
        time *= 60;
        [[fallthrough]];
    case 'm':
        time *= 60;
        [[fallthrough]];
    case 's':
        ++endp;  /* advance past the recognised suffix */
        [[fallthrough]];
    }

    if (*endp != '\0') {
        return -1; /* string continues after digits or known unit */
    }

I argue that the magic numbers here are well-known and unlikely to change, so do not need named constants.