[Swan-dev] tricky arithmetic

[Antony Antony]

On Fri, Jan 30, 2015 at 04:54:06PM -0500, D. Hugh Redelmeier wrote:
> | 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?

the spec said __suseconds_t could be -1. If that happens unsigned long will print big number? 

> 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.

I don't undersant why "long int" %ld would not be sufficient and we need "unsigned long".

> 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.

So the judgment to use unsigned is based on -1 is unresasonable and not what standard state, standard state -1 is possibilty?


> 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

only base to revert this commit is your judgement. My prefernce is long int. I happy to revert.

> | 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.

Since we are entering subsecond relam there will be lot of subsecond computations  and formatted out. Some clarity and best practice on how to store and output this would be nice.