Rather than churning all of the locations that use a 2-argument timespecfoo(), did you consider a compatibility implementation like this?

#define timespecadd_impl(arg1, arg2, arg3, arg4, ...)  do {                     \
        struct timespec *dst;                                                   \
                                                                                \
        _Static_assert((arg4) == NULL, "timespecadd: 4th argument");            \
        if ((arg3) != NULL)                                                     \
                dst = (arg3);                                                   \
        else                                                                    \
                dst = (arg1);                                                   \
                                                                                \
        dst->tv_sec = (arg1)->tv_sec + (arg2)->tv_sec;                          \
        dst->tv_nsec = (arg1)->tv_nsec + (arg2)->tv_nsec;                       \
        if (dst->tv_nsec >= 1000000000L) {                                      \
                dst->tv_sec++;                                                  \
                dst->tv_nsec -= 1000000000L;                                    \
        }                                                                       \
} while (0)

#define timespecadd(arg1, arg2, ...) do {                                       \
        timespecadd_impl((arg1), (arg2), ## __VA_ARGS__, NULL, NULL);           \
} while (0)

The changes to adapt to the new API look correct to me.

In D14725#309586, @cem wrote:

Rather than churning all of the locations that use a 2-argument timespecfoo(), did you consider a compatibility implementation like this?

#define timespecadd_impl(arg1, arg2, arg3, arg4, ...)  do {                     \
        struct timespec *dst;                                                   \
                                                                                \
        _Static_assert((arg4) == NULL, "timespecadd: 4th argument");            \
        if ((arg3) != NULL)                                                     \
                dst = (arg3);                                                   \
        else                                                                    \
                dst = (arg1);                                                   \
                                                                                \
        dst->tv_sec = (arg1)->tv_sec + (arg2)->tv_sec;                          \
        dst->tv_nsec = (arg1)->tv_nsec + (arg2)->tv_nsec;                       \
        if (dst->tv_nsec >= 1000000000L) {                                      \
                dst->tv_sec++;                                                  \
                dst->tv_nsec -= 1000000000L;                                    \
        }                                                                       \
} while (0)

#define timespecadd(arg1, arg2, ...) do {                                       \
        timespecadd_impl((arg1), (arg2), ## __VA_ARGS__, NULL, NULL);           \
} while (0)

I think this kind of compatibility should be provided only if necessary for old code that cannot be changed (such as third party code). If it is just to avoid churn, we should keep using the old timespecadd and timespecsub (that is, abandon this change).

Apart from that, this compatibility code has a run-time effect where a call like timespecadd(a, b, c) writes to *a iff c is NULL, which makes it harder to remove eventually. If compatibility is wanted, it should be a pure compile-time check such as using C11 _Generic or our __generic.

I think this kind of compatibility should be provided only if necessary for old code that cannot be changed (such as third party code). If it is just to avoid churn, we should keep using the old timespecadd and timespecsub (that is, abandon this change).

Completely agree.

In D14725#309659, @emaste wrote:

I think this kind of compatibility should be provided only if necessary for old code that cannot be changed (such as third party code). If it is just to avoid churn, we should keep using the old timespecadd and timespecsub (that is, abandon this change).

Completely agree.

I agree, too. But I *do* think that userland should have some form of timespecadd, and I think we need to follow NetBSD's example, or 3rd party programs will have even worse compatibility issues. Compared to that, changing our internal KPI isn't too hard.

In D14725#309655, @jilles wrote:

I think this kind of compatibility should be provided only if necessary for old code that cannot be changed (such as third party code). If it is just to avoid churn, we should keep using the old timespecadd and timespecsub (that is, abandon this change).

I don't follow that line of argument — that compatibility means we should abandon 3-arg timespecfoo, or bringing timespecfoo into userspace. But I will bow to quorum here, which seems to be: we'd rather churn it than have compatibility.

Apart from that, this compatibility code has a run-time effect where a call like timespecadd(a, b, c) writes to *a iff c is NULL, which makes it harder to remove eventually.

Any such code currently has undefined behavior. At best it has runtime behavior of immediately panicing, so, ¯\_(ツ)_/¯. Maybe don't write UB code. Consider also this API is usually used on immediate stack variables, rather than pointers that can sometimes be NULL.

If compatibility is wanted, it should be a pure compile-time check such as using C11 _Generic or our __generic.

Can something like this be implemented with _Generic? I don't see it, but would be curious to learn how.

In D14725#309715, @cem wrote:

In D14725#309655, @jilles wrote:

I think this kind of compatibility should be provided only if necessary for old code that cannot be changed (such as third party code). If it is just to avoid churn, we should keep using the old timespecadd and timespecsub (that is, abandon this change).

I don't follow that line of argument — that compatibility means we should abandon 3-arg timespecfoo, or bringing timespecfoo into userspace. But I will bow to quorum here, which seems to be: we'd rather churn it than have compatibility.

Moving to 3-arg timespecfoo means that we will eventually have to change all uses of 2-arg timespecfoo; I hope that is not controversial. Then the question is just whether we should eat that churn right away or do it piecemeal.

Apart from that, this compatibility code has a run-time effect where a call like timespecadd(a, b, c) writes to *a iff c is NULL, which makes it harder to remove eventually.

Any such code currently has undefined behavior. At best it has runtime behavior of immediately panicing, so, ¯\_(ツ)_/¯. Maybe don't write UB code. Consider also this API is usually used on immediate stack variables, rather than pointers that can sometimes be NULL.

In the new version, it has a behaviour that might be depended on.

If compatibility is wanted, it should be a pure compile-time check such as using C11 _Generic or our __generic.

Can something like this be implemented with _Generic? I don't see it, but would be curious to learn how.

Here is an implementation of something similar using C11 _Generic:

struct macro23_noarg;

static inline void
M_2(int a, int b, struct macro23_noarg *__unused dummy)
{
	printf("two -> %d,%d\n", a, b);
}

static inline void
M_3(int a, int b, int c)
{
	printf("three -> %d,%d,%d\n", a, b, c);
}

#define M_impl(a, b, c, d, ...) do { \
	_Generic(d, \
		struct macro23_noarg *: _Generic(c, \
			struct macro23_noarg *: M_2, \
			default: M_3 \
		)(a, b, c) \
	); \
} while(0)

#define M(a, ...) M_impl((a), __VA_ARGS__, (struct macro23_noarg *)0, (struct macro23_noarg *)0, (struct macro23_noarg *)0)

Unfortunately the expressions in a generic selection must be valid to a fair degree; selecting between MM_2(a, b) and MM_3(a, b, c) causes warnings about the MM_3 call in the MM_2 case (GCC and Clang agree). Therefore, this selects between functions.

An API client must not use the struct macro23_noarg type directly.

So it sounds like this is not a good candidate for _GENERIC. Shall I commit it as-is then? I've already updated all clients of the 2-argument versions to use the 3-argument versions.

ping. Since I can't use GENERIC, are you two satisified with my solution of replacing all 2-argument uses with the 3-argument version?

Phabricator's still working:

Still Importing...
This commit is still importing. Changes will be visible once the import finishes.