In D25700#570408, @kib wrote:

Casts do make huge difference. Proposed code matches what Linux does.

Btw why not import time_before32() and time_between{,32}() as well ?

I simply import what the new drm code uses, I'll see to import them as well tomorow or friday.
Thanks

Konstantin: Please explain why:

(int)((unsigned)(x) - (unsigned)(y)) is different from
(int)((x) - (y))

I don't get it.

I mean the outer cast should be enough! Else many more macros in this file should be fixed.

In D25700#570745, @hselasky wrote:

Konstantin: Please explain why:

(int)((unsigned)(x) - (unsigned)(y)) is different from
(int)((x) - (y))

I don't get it.

If both x and y have type unsigned or unsigned types with rank less than unsigned, then indeed there is no change. Otherwise, the proposed code behavior depends on the relative rank of x, y, and uint32_t, and x,y signess.

Manu' code does either unsigned promotion or clipping on each argument, then it performs arithmetic mod UINT32_MAX, then reinterprets the result as signed.

Your code promotes one of the argument to integer type of rank of other argument, depending on what type is wider. If one type is unsigned, then other value is promoted unsigned, and again arithmetic mod size is performed.

But if both types are signed, then arithmetic is done as signed (*) and this potentially invokes undefined behavior if the result cannot be represented as signed.

- I believe that this is slightly incorrect but lets ignore some corner case.

Konstantin, doesn't the use of subtraction "-" imply that the result is signed in C?

If clipping must be predictable by use of a (unsigned) case when compiling, then more macros in this file needs an update! Doesn't freebsd specify -fwrapv, to avoid these issues?

In D25700#570754, @hselasky wrote:

Konstantin, doesn't the use of subtraction "-" imply that the result is signed in C?

No, the result has the promoted type of left and right operands. Basically the shorter type is promoted to wider one. But there are additional rules when operands have different signess.

In D25700#570755, @hselasky wrote:

If clipping must be predictable by use of a (unsigned) case when compiling, then more macros in this file needs an update! Doesn't freebsd specify -fwrapv, to avoid these issues?

Well, clipping is orthogonal to overflow. Idea with -fwrapv is to fix already broken code, but try to not introduce new broken places.

With all that corner cases and hard-to-correctly-interpret semantic, I do not see why it is useful to deviate from Linux original.

With all that corner cases and hard-to-correctly-interpret semantic, I do not see why it is useful to deviate from Linux original.

Instead of casting, would a static inline function be better, and more clean code-wise?

In D25700#570888, @hselasky wrote:

With all that corner cases and hard-to-correctly-interpret semantic, I do not see why it is useful to deviate from Linux original.

Instead of casting, would a static inline function be better, and more clean code-wise?

I do not see much difference between the macro and an inline function for such small code fragment. My caution with the function would be that it changes evaluation of arguments, in particular, the side-effects, comparing with the Linux version.

What do you think about using "int" instead of "int32" ?

@manu: What do you think about implementing the set of time_xxx functions we have as time_xxx_32() functions?

Even though you don't need them right now, we will have a consistent set of macros there?

In D25700#570955, @hselasky wrote:

What do you think about using "int" instead of "int32" ?

This is a question about required semantic of the Linux function. If (since ?) they require int32_t result, we should be explicit and not rely on the fact that all our current platforms have int32_t same as int.

In D25700#570964, @hselasky wrote:

@manu: What do you think about implementing the set of time_xxx functions we have as time_xxx_32() functions?

Even though you don't need them right now, we will have a consistent set of macros there?

I'll have a look at the list.

In D25700#571075, @kib wrote:

In D25700#570955, @hselasky wrote:

What do you think about using "int" instead of "int32" ?

This is a question about required semantic of the Linux function. If (since ?) they require int32_t result, we should be explicit and not rely on the fact that all our current platforms have int32_t same as int.