[Asterisk-code-review] func_math: Three new dialplan functions (asterisk[16])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/15906 )

Change subject: func_math: Three new dialplan functions
......................................................................


Patch Set 7: Code-Review-1

(7 comments)

https://gerrit.asterisk.org/c/asterisk/+/15906/7/funcs/func_math.c 
File funcs/func_math.c:

https://gerrit.asterisk.org/c/asterisk/+/15906/7/funcs/func_math.c@502 
PS7, Line 502: 	if (ast_strlen_zero(args.num1) || ast_str_to_int(args.num1, &int1)) {
Now that I think some more about it I think this check would also be better split out. If args.num1 is not given (ex: MIN(,5)) then continue. However if a given value fails to parse to an int maybe that should cause a return error?


https://gerrit.asterisk.org/c/asterisk/+/15906/7/funcs/func_math.c@506 
PS7, Line 506: 	if (ast_strlen_zero(args.num2) || ast_str_to_int(args.num2, &int2)) {
Similar here. Should the parsing of the int cause a separate return error?


https://gerrit.asterisk.org/c/asterisk/+/15906/7/funcs/func_math.c@514 
PS7, Line 514: 	response_int = (int1 > int2) ? int2 : int1;
What happens here if args.num1 is not set, but args.num2 is? For example MIN(,5).

Probably needs an extra check:
if response_int == -1: response_int = int2
else: response_int = (int1 > int2) ? int2 : int1;


https://gerrit.asterisk.org/c/asterisk/+/15906/7/funcs/func_math.c@532 
PS7, Line 532: 	if (ast_strlen_zero(args.num1) || ast_str_to_int(args.num1, &int1)) {
Similar to the others, should an int parsing error cause the function to return error here too?


https://gerrit.asterisk.org/c/asterisk/+/15906/7/funcs/func_math.c@536 
PS7, Line 536: 	if (ast_strlen_zero(args.num2) || ast_str_to_int(args.num2, &int2)) {
Again, should an int parsing error be separated out here too and the function return?


https://gerrit.asterisk.org/c/asterisk/+/15906/7/funcs/func_math.c@544 
PS7, Line 544: 	response_int = (int1 < int2) ? int2 : int1;
Similar to the problem above...

What happens here if args.num1 is not set, but args.num2 is? For example MAX(,5).

Probably needs an extra check:
if response_int == -1: response_int = int2
else: response_int = (int1 < int2) ? int2 : int1;


https://gerrit.asterisk.org/c/asterisk/+/15906/7/funcs/func_math.c@562 
PS7, Line 562: 		ast_log(LOG_WARNING, "Missing argument for number(s).");
Maybe update to say something like "Bad or missing argument for number" to encompass the potential parsing failure as well.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15906
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I2bda9269d18f9d54833c85e48e41fce0e0ce4d8d
Gerrit-Change-Number: 15906
Gerrit-PatchSet: 7
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-CC: Ivan Poddubny <ivan.poddubny at gmail.com>
Gerrit-Comment-Date: Thu, 20 May 2021 15:52:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210520/d56212ea/attachment-0001.html>