[asterisk-dev] [Code Review] 4159: chan_sip: Add missing braces on if, else, while and for

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4159/#review13723
-----------------------------------------------------------


I fall into the camp of being not in favor of this patch. My main concerns with this are

1) People that have their own patches they maintain against chan_sip will likely have a lot of work to do to integrate their current patches with this changeset. While people who maintain patches are likely used to having to fiddle with the patches as parts of chan_sip change, this changeset touches the entire file and likely invalidates all of their current patches.
2) If we fix a bug in chan_sip in 11 or 12, then merging the change up to 13 and/or trunk is almost certainly going to have to be done manually.

I don't think the maintenance headache that this introduces is worth the potential benefits it introduces for creating new content. If the main concern with not having braces present is that it allows for mistakes to be made when writing new patches, then I suggest that braces can be added to the sections as they are modified to have new content. If you're modifying a section of code anyway, it's fine to add missing braces there. I also think that review board will assist with the potential pitfalls since hopefully people looking at the reviews will notice if something incorrect is being done regarding braces.

- Mark Michelson


On Nov. 10, 2014, 6:22 a.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4159/
> -----------------------------------------------------------
> 
> (Updated Nov. 10, 2014, 6:22 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24510
>     https://issues.asterisk.org/jira/browse/ASTERISK-24510
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Thank you in advance to anyone who takes the time to review this tedious patch.  It is large but benign.
> 
> Scanned for / fixed instances of the regex search pattern (else|(if|while|for)\s*\(.*\))(\s*/\*.*\*/|)\s*$ in channels/chan_sip.c and channels/sip/*.c.
> 
> I've proposed this change for 13 and trunk.  The set of braces for one 'if' statement was excluded from this change to allow the entire patch to apply cleanly to 13 and trunk with patch fuzz level 0.  If you approve this change please note if you are approving it for 13+, or just trunk.  I was late to AstriDevCon so I'm not sure if this is acceptable to 13, figured it doesn't hurt to ask. Asterisk 13 will be supported for a long time and chan_sip is in extended support status.  I feel that adding the braces will make it easier/lower risk for me to fix bugs that are found in the next 4 years.
> 
> I have no idea what reviewboard is doing with the display for lines 6078 - 6254, no code is actually moved.  Check the diff file for the actual changes in that section.
> 
> 
> Diffs
> -----
> 
>   /branches/13/channels/sip/reqresp_parser.c 427641 
>   /branches/13/channels/sip/dialplan_functions.c 427641 
>   /branches/13/channels/sip/config_parser.c 427641 
>   /branches/13/channels/chan_sip.c 427641 
> 
> Diff: https://reviewboard.asterisk.org/r/4159/diff/
> 
> 
> Testing
> -------
> 
> Compiled, visually inspected.  Ran all of tests/channel/SIP in testsuite.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20141111/7220cf46/attachment-0001.html>