Summary:ASTERISK-04477: [change request] swap arguments to ast_tvdiff_ms()
Reporter:Luigi Rizzo (rizzo)Labels:
Date Opened:2005-06-24 10:04:24Date Closed:2005-07-11 16:39:43
Versions:Frequency of
Description:ast_tvdiff_ms(a, b) returns the difference in milliseconds between its arguments.
        So would you expect it to compute (a - b) or (b - a) ?
Guess what, it does (b - a), which is contrary to all other instances
of similar functions in asterisk, to (i think) common expectations,
and to the code i submitted in ASTERISK-4388 :)

I am ok with a different name, but i would really like to swap the args.
Given that this is used only once in res/res_features.c,
it should not be a major issue -- it would be much worse and especially
more error-prone to fix all other usages.


BTW what do we want to do with ASTERISK-4388 ? Do you want me to resubmit
the patch putting the possibly-inline code into utils.h and using
ast_tvdiff_ms() (with swapped args) in place of mine ast_tvdiff()?
Comments:By: Kevin P. Fleming (kpfleming) 2005-06-24 12:58:36

It's only unclear when you post it that way, vs. the way it actually looks in the code:

* \brief Computes the difference (in milliseconds) between two \c struct \c timeval instances.
* \param start the beginning of the time period
* \param end the end of the time period
* \return the difference in milliseconds
int ast_tvdiff_ms(const struct timeval *start, const struct timeval *end);

If you actually read the docs and/or the prototype, it's very clear how it works.

Now, I'm not opposed to changing the order at this point, if there are corresponding other APIs we should match. Off the top of my head I'm not aware of any, but I'm willing to be convinced otherwise :-)

By: Luigi Rizzo (rizzo) 2005-06-24 14:28:01

if you look at the patch in ASTERISK-4388, there were already existing
similar functions (with the arguments the way i like) in
channel.c                       @@ -2853,8 +2833,7 @@
                               @@ -3022,8 +3001,7 @@
apps/app_alarmreceiver.c        @@ -249,14 +235,12 @@
apps/app_disa.c                 @@ -193,9 +183,8 @@

not much but it is still 4-to-1 -- i win :)

I haven't found a timeval_diff() function of some kind, but difftime(a, b)
seems to be some standard and also does a-b

By: Tilghman Lesher (tilghman) 2005-06-27 15:13:03

I disagree.  When I want a time diff, I normally think that the parameters should be in chronological order, which is what they are already.

There isn't much of an issue here:  it's documented, and it works the way it is documented.  Nuff said.

By: Luigi Rizzo (rizzo) 2005-06-27 18:48:59

I am just reporting that asterisk has only one instance (with one use)
of ast_tvdiff_ms(then, now), while it has three implementations
(used four times in total) of equivalent functions
joe_smith_version_of_time_diff_ms(now, then), not to mention the almost 100
instances of inlined code naturally written as
diff= (now.tv_sec - then.tv_sec) * 1000 + (now.tv_usec - then.tv_usec)/1000
I didn't write any of that code, but the common usage seems to be now - then.

Surely you can write and document something, pretend it is _the_ standard
and ignore the status quo.

But POLA is typically a good principle in sw engineering
and it seems to me that we are going against it.

By: Kevin P. Fleming (kpfleming) 2005-07-11 16:39:24

I've reversed the arguments, on the basis that they will now match the underlying math, rather than that they match any other particular APIs.

I will address your inlining macro that you've posted, then you can repost ASTERISK-4474504 taking into account the new stuff in the tree. Thanks for your patience!