Summary:	ASTERISK-04477: [change request] swap arguments to ast_tvdiff_ms()
Reporter:	Luigi Rizzo (rizzo)	Labels:
Date Opened:	2005-06-24 10:04:24	Date Closed:	2005-07-11 16:39:43
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:
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. ****** ADDITIONAL INFORMATION ****** 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!