<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, 3:38 p.m. CDT, <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>
<p>On May 30th, 2014, 4:10 p.m. CDT, <b>George Joseph</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="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>
</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;">For now this split should be done without regard to the anticipated weak reference feature. The weak reference design may change radically again before it is workable and any change in anticipation may not be desirable as ao2 is currently implemented. For example, have you considered keeping a pointer to the container instead of keeping a pointer to the internal container node in the weak reffed object? The big advantage would be a much reduced need for internal container knowledge.
The best split could be:
astobj2.c - object primitives, object primitive misc and initialization code.
astobj2_private.h - internal object primitive declarations needed by the containers. (May just be folded into the astobj2_container_private.h file)
astobj2_container.c - generic conainer and container misc code (You may not want to go this far because of the reasons below)
astobj2_container_hash.c - hash container specific code
astobj2_container_rbtree.c - rbtree container specific code
astobj2_container_private.h - generic container definitions and rtti prototypes (Possibly misc internal ao2 primitive definitions if not that many)
I don't think separating the generic container code and the misc code would be as easy. The misc code would need to be split between the ao2 object primitive file and the generic container code file. I expect the misc code to have a lot more implementation dependencies that would be harder to resolve.</pre>
<br />
<p>- rmudgett</p>
<br />
<p>On May 29th, 2014, 7:53 p.m. CDT, 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, 7: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>