[Home]

Summary:ASTERISK-14060: [patch] Functions INC and DEC
Reporter:Nir Simionovich (GreenfieldTech - Israel) (greenfieldtech)Labels:
Date Opened:2009-05-03 20:30:27Date Closed:2009-06-01 15:34:27
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Functions/func_math
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) func_math.c.patch
( 1) func_math.c.patch_v2
( 2) func_math.c.patch_v3
( 3) func_math.c.patch_v4
( 4) func_math.c.patch-incdec
( 5) strings.h.patch
( 6) strings.h.patch.v2
( 7) strings.h.patch-is_numeric
Description:Over the course of time, developing Asterisk dialplans becomes fairly cumbersome, especially when writing While() loops in the dialplan. Mainly, when we want to iterate a few times, the ever annoying Set(Var=$[${Var} + 1]) is really annoying.
INCrement and DECrement follow the old PASCAL functions, allowing to increment and decrement a variable.

The functions are built to handle both numeric and alpha-numeric values. If an alpha-numeric value is given, the functions will simply bail out and issue a LOG_NOTICE accordingly.

In order to implement this, i've added some functionality to func_math.c and added a new ast_is_numeric function to strings.h

Please find attached patches for TRUNK.

****** ADDITIONAL INFORMATION ******

The below dialplan illustrates the usage of these functions within a dialplan:

[incdec]
exten => _X.,1,Answer
exten => _X.,n,Wait(3)
exten => _X.,n,Set(MyVar=10)
exten => _X.,n,Noop(${INC(MyVar)})
exten => _X.,n,Noop(MyVAR is Now: ${MyVar})
exten => _X.,n,Noop(${DEC(MyVar)})
exten => _X.,n,Noop(MyVAR is Now: ${MyVar})
exten => _X.,n,While($[${DEC(MyVar)} > 0])
exten => _X.,n,Noop(MyVAR is Now: ${MyVar})
exten => _X.,n,EndWhile
exten => _X.,n,Noop(${INC(NoVar)})
exten => _X.,n,Set(NewVar=ab)
exten => _X.,n,Noop(${INC(NewVar)})
exten => _X.,n,Noop(NewVAR is Now: ${NewVar})
Comments:By: Tilghman Lesher (tilghman) 2009-05-03 23:19:42

You may want to put up your changes on reviewboard.asterisk.org, as there are multiple changes that I have to suggest, and that would be an easier place to give those suggestions.
By: Atis Lezdins (atis) 2009-05-05 04:58:03

It seems that You're using some bad editor, as there are some spacing changes.

You can do diff -w to ignore whitespace changes :)

Also, i would suggest not using MyVar in doc, but something in lowercase - like <variable>, and the same name in syntax warning (of which i'm still not sure that it should be warning :)

Btw, looking at the code of ast_is_numeric - does this work with negative numbers? Are numbers >2^31 allowed in dialplan variables? If not, i would go with simpler checking.
By: Nir Simionovich (GreenfieldTech - Israel) (greenfieldtech) 2009-05-05 06:14:32

That's odd, I used VIM to do that ... Let me get the code into my Eclipse CDT and I'll see if that helps out.

I'll change the doc from MyVar to variable - thanks.

Negative is ast_is_numeric ... Ahhh ... (mental note to self, at 3am go to bed).
By: Nir Simionovich (GreenfieldTech - Israel) (greenfieldtech) 2009-05-05 08:23:48

Ok, I've uploaded a new version for strings.h, to take into consideration the fact that numbers can also be negative.
By: Nir Simionovich (GreenfieldTech - Israel) (greenfieldtech) 2009-05-05 08:42:42

please disregards the func_math.c.patch file, the func_math.c.patch_v2 file is the correct one, with added documentation.
By: Atis Lezdins (atis) 2009-05-05 09:06:47

Well, it looks better, but there's still inconsistency of naming between XML and WARNING :)

ast_log(LOG_WARNING, "Syntax: %s(<data>) - missing argument!\n",cmd);
By: Nir Simionovich (GreenfieldTech - Israel) (greenfieldtech) 2009-05-05 09:53:01

corrected
By: Nir Simionovich (GreenfieldTech - Israel) (greenfieldtech) 2009-05-11 15:08:11

Any feedback?
By: Tilghman Lesher (tilghman) 2009-05-12 03:56:27

There's no need to use ast_strdupa() on the value if the channel is locked the entire time.  The only reason to ast_strdupa() is if you immediately unlock the channel and you want your pointer to remain valid.

All conditionals need braces around the body, even when the body is only a single line.  This is part of our coding guidelines.

There should be a space after every comma in a multi-parameter function, such as snprintf().

Comments on the same line as code should be indented with spaces, not tabs.  Tabs should only be used for indenting at the very beginning of a line.

The returnvar only needs to be 12 characters long if you're using an int type.  It could be 23 characters long if you're using a long long type, though.  32 is way overkill.  If you want to use a calculation based upon the type, you can always use:  char returnvar[3 + 2.4083 * sizeof(int)]
By: Nir Simionovich (GreenfieldTech - Israel) (greenfieldtech) 2009-05-13 05:08:47

Hmmmm... valid points. i'll work this out.

I was under the impression that ast_strdupa() requires me to lock the channel prior to using it, but if that isn't the case, I'll remove the locking.
By: Tilghman Lesher (tilghman) 2009-05-13 10:09:36

Yes, ast_strdupa does require you to lock the channel prior to using it.  The point was that as the patch locks the channel for the entire time that the duplicated string was in use, the ast_strdupa was not necessary (the original string would have remained protected, since the channel was still locked).  So you should either a) eliminate the ast_strdupa or b) reduce the time the channel is locked to JUST the time that the lock is needed.
By: Nir Simionovich (GreenfieldTech - Israel) (greenfieldtech) 2009-05-15 15:22:49

Hi Guys,

 New patch uploaded with fixes. I've decided to remove the strdupa and remain with the locks. As I'm dealing with variables, I would like to maintain the lock by myself and verify it's there till I finish the thing.

 I've also added another check into the mix, to verify that the functions always unlocks, even in the case of an error.
By: Tilghman Lesher (tilghman) 2009-05-18 11:50:04

Why prevent INC and DEC from running on negative numbers?
By: Nir Simionovich (GreenfieldTech - Israel) (greenfieldtech) 2009-05-20 07:25:20

New patch for strings.h submitted for handling negative numbers, thanks for the catch on that one.
By: Nir Simionovich (GreenfieldTech - Israel) (greenfieldtech) 2009-05-25 04:41:16

Review anyone?
By: Leif Madsen (lmadsen) 2009-05-26 09:36:05

Is this currently on reviewboard? If so, it might be useful to provide the link to the review. Thanks!
By: Leif Madsen (lmadsen) 2009-05-26 09:38:28

Not sure if this is just a typo, or how you are actually wanting this to work, but I don't like this format in the dialplan:

exten => _X.,n,While($[DEC(MyVar) > 0])

I'd expect to do:

exten => _X.,n,While($[${DEC(MyVar)} > 0])

The ${DEC(MyVar)} should decrement the number from ${MyVar} and return the result.

Like I said, this may be exactly how it works now, but could just be a typo in the dialplan example.

BTW: As someone who does a lot of loops, I like this!
By: Nir Simionovich (GreenfieldTech - Israel) (greenfieldtech) 2009-05-27 14:56:20

Hmmm... either I did something wrong, right, or just hit a configuration parser bug. I just tried changing the format of the call from the previous one (which worked just one) to the one Lief just proposed - it worked exactly the same!

Does that make any sense?
By: Nir Simionovich (GreenfieldTech - Israel) (greenfieldtech) 2009-05-31 15:44:20

After discussing this on the IRC, Lief's remarks were nullified by tilghman.
By: Digium Subversion (svnbot) 2009-06-01 15:33:51

Repository: asterisk
Revision: 198725

U   trunk/funcs/func_math.c

------------------------------------------------------------------------
r198725 | tilghman | 2009-06-01 15:33:50 -0500 (Mon, 01 Jun 2009) | 8 lines

Add INCrement and DECrement functions
(closes issue ASTERISK-14060)
Reported by: greenfieldtech
Patches:
      func_math.c.patch_v4 uploaded by greenfieldtech (license 369)
      slightly modified by me
Tested by: greenfieldtech, lmadsen

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=198725
By: Digium Subversion (svnbot) 2009-06-01 15:34:26

Repository: asterisk
Revision: 198726

_U  branches/1.6.2/

------------------------------------------------------------------------
r198726 | tilghman | 2009-06-01 15:34:26 -0500 (Mon, 01 Jun 2009) | 14 lines

Blocked revisions 198725 via svnmerge

........
 r198725 | tilghman | 2009-06-01 15:33:50 -0500 (Mon, 01 Jun 2009) | 8 lines
 
 Add INCrement and DECrement functions
 (closes issue ASTERISK-14060)
  Reported by: greenfieldtech
  Patches:
        func_math.c.patch_v4 uploaded by greenfieldtech (license 369)
        slightly modified by me
  Tested by: greenfieldtech, lmadsen
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=198726