<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Patch Set 3:<br>> What's the difference between<br>${EXTENSION_STATE(${parking_lot}@${parking_space})} = INUSE<br>and<br>${PARK_OCCUPIED(${parking_space},${parking_lot})}<br>?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">I think the difference there is that EXTENSION_STATE would require<br>that the parking lot extension be hinted. I don't know that's<br>guaranteed to be the case all the time. Might be some other odd<br>state considerations there I'd have to think through, but this at<br>least seemed to keep it fairly clean for us.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Would you guys prefer removing PARK_OCCUPIED in favor of relying on<br>EXTENSION_STATE?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">If the EXTENSION_STATE version is sufficient then I lean toward that. Could you also not get similar from PARK_GET_CHANNEL too? If that function returns a channel name then it's occupied, otherwise if it returns nothing then the slot would be been empty/unoccupied correct?</p><p>Patch set 3:<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/9375">View Change</a></p><p>3 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9375/3/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/9375/3/res/parking/parking_bridge_features.c@820">Patch Set #3, Line 820:</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 (!ast_strlen_zero(args.parking_space)) {<br> if (ast_str_to_uint(args.parking_space, &space) != 0) {<br> ast_log(LOG_ERROR, "value '%s' for parking_space argument is invalid. Must be an integer greater than 0.\n", args.parking_space);<br> return -1;<br> }<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This section is using spaces instead of tabs. Please use tabs for indentions.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9375/3/res/parking/parking_controller.c">File res/parking/parking_controller.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/9375/3/res/parking/parking_controller.c@168">Patch Set #3, Line 168:</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;"> RAII_VAR(struct parked_user *, user, NULL, ao2_cleanup);<br><br> if (target < 0) {<br> user = ao2_callback(lot->parked_users, 0, NULL, NULL);<br> } else {<br> user = ao2_callback(lot->parked_users, 0, retrieve_parked_user_targeted, &target);<br> }<br><br> if (!user) {<br> return NULL;<br> }<br><br> /* Bump the ref count by 1 since the RAII_VAR will eat the reference otherwise */<br> ao2_ref(user, +1);<br> return user;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Remove the RAII_VAR use here as it adds extra overhead in this case. You can then also remove the ref bump at the end. Actually at that point this whole thing could be collapsed into a single line (if you wanted to that is):</p><p style="white-space: pre-wrap; word-wrap: break-word;">return target < 0 ? ao2_callback(....) : ao2_callback(...)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/9375/3/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/9375/3/res/parking/res_parking.h@199">Patch Set #3, Line 199:</a> <code style="font-family:monospace,monospace"> * \since 12.0.0</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Remove this since this method is being added now.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/9375">change 9375</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/9375"/><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: Idba6ae55b8a53f734238cb3d995cedb95c0e7b74 </div>
<div style="display:none"> Gerrit-Change-Number: 9375 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Joshua Elson <joshelson@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Alexei Gradinari <alex2grad@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Elson <joshelson@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 09 Jul 2018 19:30:04 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>