[Asterisk-code-review] func_groupcount.c: Adding Group Variables and additional Group functions (asterisk[18])

[Mark Murawski]

Attention is currently required from: Sean Bright, N A.

Mark Murawski has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/17655 )

Change subject: func_groupcount.c: Adding Group Variables and additional Group functions
......................................................................


Patch Set 15:

(13 comments)

Patchset:

PS13: 
> Additionally, -1 pending resolution of https://issues.asterisk.org/jira/browse/ASTERISK-30221 […]
Done


Patchset:

PS14: 
> Also: this needs a CHANGES entry
Done


File funcs/func_groupcount.c:

https://gerrit.asterisk.org/c/asterisk/+/17655/comment/7e4ecd10_a2c21be6 
PS8, Line 87: 						<para>Inherit. Group membership will be kept when a channel is transferred.</para>
> I see - at the very least, including that explanation in the description would be good.
Done


File funcs/func_groupcount.c:

https://gerrit.asterisk.org/c/asterisk/+/17655/comment/f7331906_63a1d16f 
PS13, Line 1302: 	ast_str_append(&out, 0, "%d active channel%s in groups\n", numchans, ESS(numchans));
> This is inaccurate. It should be something like "active group assignments". […]
Done


File funcs/func_groupcount.c:

https://gerrit.asterisk.org/c/asterisk/+/17655/comment/229015af_ba38e1f0 
PS14, Line 28: #include <regex.h>
> By convention, asterisk.h always goes first. […]
Done


https://gerrit.asterisk.org/c/asterisk/+/17655/comment/43dc605c_5fd29c90 
PS14, Line 593: 	buf[0] = 0;
> Use '\0' for semantics, not 0
Done


https://gerrit.asterisk.org/c/asterisk/+/17655/comment/9abeb3a1_89cbf785 
PS14, Line 600: 	if (ast_app_group_split_group(args.groupcategory, group, sizeof(group), category, sizeof(category)))
> This needs braces for single line ifs, per the coding guidelines
Done


https://gerrit.asterisk.org/c/asterisk/+/17655/comment/d0690f7c_fc0d8b5e 
PS14, Line 783:         const char *group_match    = NULL;
> Whitespace should be tabs here
Done


https://gerrit.asterisk.org/c/asterisk/+/17655/comment/8015ff74_bff4d199 
PS14, Line 795: 
> Whitespace/alignment/indentation also messed up here
I don't follow this one... are we enforcing alignment on every single variable now?  I didn't see the need to line up anything here as everything in this little block was so dissimilar.


File include/asterisk/channel.h:

https://gerrit.asterisk.org/c/asterisk/+/17655/comment/8d2656c7_c365d3ad 
PS14, Line 2931: 	struct varshead varshead;			/*!< A linked list for group variables. See \ref AstGroupVar */
> Remove one tab to align the comments
I don't think I see an issue here.   The comments are already lined up with tabs.


https://gerrit.asterisk.org/c/asterisk/+/17655/comment/330a0019_f361e5cd 
PS14, Line 3164:  * \since 20
> I don't think \since 20 is accurate and can be removed. This will likely go into 18 as well.
Done


File main/app.c:

https://gerrit.asterisk.org/c/asterisk/+/17655/comment/ce5da971_07e6ec81 
PS14, Line 2433: 
> Whitespace issues again here
Done


https://gerrit.asterisk.org/c/asterisk/+/17655/comment/51ab4586_2396cce7 
PS14, Line 2686: 		strncpy(gmi->group, group, (MAX_GROUP_LEN - 1));
> Use ast_copy_string, not strncpy
Done



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

Gerrit-Project: asterisk
Gerrit-Branch: 18
Gerrit-Change-Id: I23e48d1cdfc8adaffdfec2e936e56143603914f2
Gerrit-Change-Number: 17655
Gerrit-PatchSet: 15
Gerrit-Owner: Mark Murawski <markm at intellasoft.net>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: N A <mail at interlinked.x10host.com>
Gerrit-Comment-Date: Mon, 26 Sep 2022 15:26:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: N A <mail at interlinked.x10host.com>
Comment-In-Reply-To: Mark Murawski <markm at intellasoft.net>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220926/21402e6e/attachment.html>