<p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/9933">View Change</a></p><p>11 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9933/2/main/cdr.c">File main/cdr.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9933/2/main/cdr.c@1612">Patch Set #2, Line 1612:</a> <code style="font-family:monospace,monospace"> char park_info[AST_MAX_EXTENSION]; /* ideally: max. parking lot strlen + max. digits of ast_parkingspace_int */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">AST_MAX_EXTENSIONS is defined as 80 so you have needlessly shrunk the size of this buffer and potentially caused truncation where it didn't happen before.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9933/2/res/parking/parking_applications.c">File res/parking/parking_applications.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9933/2/res/parking/parking_applications.c@40">Patch Set #2, Line 40:</a> <code style="font-family:monospace,monospace">/*** DOCUMENTATION</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The XML documentation changes here should go in its own patch since they have nothing to do with this new feature and should go into the other branches.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9933/2/res/parking/parking_bridge.c">File res/parking/parking_bridge.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9933/2/res/parking/parking_bridge.c@127">Patch Set #2, Line 127:</a> <code style="font-family:monospace,monospace"> preferred_space = ((ast_parkingspace_int) ast_random() * (ast_parkingspace_int) ast_random()) % (lot->cfg->parking_stop - lot->cfg->parking_start + 1);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Only one call to ast_random is needed. We are determining the offset into the parking lot at this time. Besides you cannot have a parking lot with more than 2^32 spaces as you would have run out of memory long before that.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9933/2/res/parking/parking_bridge_features.c">File res/parking/parking_bridge_features.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9933/2/res/parking/parking_bridge_features.c@692">Patch Set #2, Line 692:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> sprintf(buf, AST_PARKINGSPACE_FORMAT, numeric_value);<br> ast_say_digit_str(bridge_channel->chan, buf, "", ast_channel_language(bridge_channel->chan));<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should use your parking_space_announce() macro here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9933/2/res/parking/res_parking.h">File res/parking/res_parking.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9933/2/res/parking/res_parking.h@63">Patch Set #2, Line 63:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ast_parkingspace_int parking_start; /*!< First space in the parking lot */<br> ast_parkingspace_int parking_stop; /*!< Last space in the parking lot */<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The EOL comments should be made to line up again.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9933/2/res/parking/res_parking.h@90">Patch Set #2, Line 90:</a> <code style="font-family:monospace,monospace"> ast_parkingspace_int next_space; /*!< When using parkfindnext, which space we should start searching from next time we park */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The EOL comments should be made to line up again.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9933/2/res/parking/res_parking.h@106">Patch Set #2, Line 106:</a> <code style="font-family:monospace,monospace"> ast_parkingspace_int parking_space; /*!< Which parking space is used */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The EOL comments should be made to line up again.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9933/2/res/res_parking.c">File res/res_parking.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9933/2/res/res_parking.c@453">Patch Set #2, Line 453:</a> <code style="font-family:monospace,monospace"> return left->parking_space - right->parking_space;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You are going to have to make this comparison more complicated as you cannot return a long long difference here.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Fortunately you just need to return the standard strcmp() return meanings.<br>left < right if return value is less than zero<br>left = right if return value is zero<br>left > right if return value is greater than zero</p><p style="white-space: pre-wrap; word-wrap: break-word;">so you could return -1, 0, 1 as needed.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9933/2/res/res_parking.c@819">Patch Set #2, Line 819:</a> <code style="font-family:monospace,monospace"> snprintf(space, sizeof space, AST_PARKINGSPACE_FORMAT, parkingspace);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Keep the parentheses with sizeof. It really looks weird without them and they are required with some uses.</p><p style="white-space: pre-wrap; word-wrap: break-word;">sizeof(space)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9933/2/res/res_parking.c@827">Patch Set #2, Line 827:</a> <code style="font-family:monospace,monospace"> arguments_string = ast_str_create(strlen(lot_cfg->name) + 1 + sizeof space);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Revert this change. This is the initial size of the created ast_str. It will dynamically grow as needed.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9933/2/res/res_parking.c@834">Patch Set #2, Line 834:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (parking_add_extension(lot_context, 0, space, 1, PARKED_CALL_APPLICATION, ast_str_buffer(arguments_string), parkedcall_registrar_pointer)<br> ) {<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Revert this change. Coding guidelines want lines wrapped at 90 columns.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/9933">change 9933</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/9933"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ifbeee7546d5ab3ea3ce29c68b4810eb4e51decad </div>
<div style="display:none"> Gerrit-Change-Number: 9933 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Karl Brose <karlbrose@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 22 Aug 2018 19:26:32 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>