[Swan-dev] tricky arithmetic

[D. Hugh Redelmeier]

| From: Antony Antony <antony at phenome.org>
| 
| On Wed, Jan 28, 2015 at 06:21:58PM -0500, D. Hugh Redelmeier wrote:
| > tv_diff = (tv2.tv_sec - tv1.tv_sec) * 1000000 + (tv2.tv_usec - tv1.tv_usec);
| > tv_diff = tv_diff != 0.0 ? tv_diff/1000 : 0;
| > 
| > I assumed that .tv_usec was an unsigned type.  So
| > 	tv2.tv_usec - tv1.tv_usec
| > would wrap around in a binary fashion if
| > 	tv2.tv_usec < tv1.tv_usec
| > This would produce the wrong result.
| > 
| > It turns out that .tv_usec is defined by POSIX to be of type
| > suseconds_t and
| > 	"The type suseconds_t shall be a signed integer type capable
| > 	of storing values at least in the range [-1, 1000000]."
| > This is a wierd type for C.  Any normal C signed type capable of
| > representing 1000000 is also capable of representing -1000000.
| 
| In C tv_usec is signed int, then while printing why not cast the difference to int?
| 
| -               DBG_log("elapsed time in %s for hostname lookup %lu.%06lu",
| +               DBG_log("elapsed time in %s for hostname lookup %lu.%06d",
|                         __func__,
|                         (unsigned long)(tv2.tv_sec - borrow - tv2.tv_sec),
| -                       (unsigned long)(tv2.tv_usec + borrow * 1000000 - tv2.tv_
| +                       (int)(tv2.tv_usec + borrow * 1000000 - tv2.tv_usec));  
| 
| Or should it %ld? to be safe. Some compilers may use long int for __suseconds_t?
| 
| Without cast there is a warning. I wonder why derived type should be 
| casted:) I found this annoying side effects to too much typdef.  It is 
| also easy to make mistake such as this you cast to wrong type. Or we we 
| need function as we do with deltaseconds.

Why did you change it?

I think that it worked, without diagnostic, before you changed it.  (I
admit: I didn't test it.)

You have no reasonable way of knowing what the underlaying type of
suseconds_t is.  It is not always int since int might not be able to
represent 1000000.  Changing it to int is a bug.

On some C implementations, the type "int" is too small too represent
1000000.  So you know that suseconds_t might not be int.

suseconds_t may or may not be long, but long is large enough to
represent all values.

In this case, unsigned long can also represent all values.  And it is
simpler to understand because a negative value would not be
reasonable and must not occur.

For similar reasons, changing the type of "borrow" to int is wrong
too:
	borrow * 1000000  <==== can overflow

|  warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘__suseconds_t’ [-Wformat=] 

It is being very kind to you: reporting a portability error.

I suggest reverting 87590de5401c0e0bd4eaf93bb02c68eca544990a

| float warning to Hugh:) In this case using a double could make the code easier to read! 

Yes.

I don't really care for using floating point when it isn't necessary
or natural.  But natural is in the eye of the beholder.