<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3576/">https://reviewboard.asterisk.org/r/3576/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 30th, 2014, 2:38 p.m. MDT, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/3576/diff/1/?file=59035#file59035line156" style="color: black; font-weight: bold; text-decoration: underline;">branches/12/main/astobj2_private.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">156</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">enum</span> <span class="n">ao2_callback_type</span> <span class="p">{</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">157</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">AO2_CALLBACK_DEFAULT</span><span class="p">,</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">158</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="n">AO2_CALLBACK_WITH_DATA</span><span class="p">,</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">159</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="p">};</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">There is too much in this file that should not be here.
1) Almost everything from this enum definition and above should stay where it was in astobj2. It does not need to be spread outside of that file.
2) The INTERNAL_OBJ() references in the container code is easily abstracted to an internal get the ao2_options value API call.
3) The hash container specific declarations in this file should be only in the astobj2_hash.c file. The same should be for the rbtree declarations respectively.
4) About the only stuff remaining in this file should be some miscellaneous function prototypes and generic container and node definitions common to all containers.
ao2 containers are built on top of ao2 objects. I laid out the astobj2.c file into sections which for the most part could be easily split into their own files.
1) astobj2 object primitives code
2) astobj2 container generic code
3) astobj2 container hash specific code
4) astobj2 container rbtree specific code
5) astobj2 miscellaneous code (unit test, CLI debug commands, initialization)
Reference https://reviewboard.asterisk.org/r/2110/ for mmichelson's comment about breaking astobj2.c up. I've covered most of the contents of the exchange here.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The reason I moved the hash-specific stuff into the common header file is that the weak-reference implementation re-uses a good chunk of the hash implementation...all of the structures and most of the code. The rbtree stuff was moved just to be consistent. Knowing that, what would you like me to do?
Do you want me to further break out the container common code into astobj2_container.c and the misc code into astobj2_misc.c?
</pre>
<br />
<p>- George</p>
<br />
<p>On May 29th, 2014, 6:53 p.m. MDT, George Joseph wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers and rmudgett.</div>
<div>By George Joseph.</div>
<p style="color: grey;"><i>Updated May 29, 2014, 6:53 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">In preparation for weak-reference containers, and because it makes the existing code easier to read and maintain, I've split the astobj2 common structure and enum definitions and prototypes into astobj2_private.h, the hash table implementation into astobj2_hash.c, and the rbtree implementation into astobj2_rbtree.c. All of the public functions remain in astobj2.c.
A few functions (adjust_lock, container_destruct, container_destruct_debug) needed to have their static modifiers removed so they'd be visible from the other object files but other than that there were NO functional changes, no logic changes, etc.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I used both the test framework and the test suite. For the test suite, I used channels/pjsip since that exercises sorcery significantly and that in turn exercises astobj2.
All tests that worked before the change worked after the change.
Before...
Test Framework
393 Test(s) Executed 393 Passed 0 Failed
Test Suite
tests/channels/pjsip/
Tests: 88 Passed: 87 Failed: 1
FAILED: tests/channels/pjsip/dialplan_functions/pjsip_endpoint
After...
Test Framework
393 Test(s) Executed 393 Passed 0 Failed
Test Suite
tests/channels/pjsip/
Tests: 88 Passed: 87 Failed: 1
FAILED: tests/channels/pjsip/dialplan_functions/pjsip_endpoint
Not sure why the pjsip_endpoint function is failing but it's not this patch's fault.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>branches/12/main/astobj2_rbtree.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>branches/12/main/astobj2_private.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>branches/12/main/astobj2_hash.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>branches/12/main/astobj2.c <span style="color: grey">(414877)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3576/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>