[asterisk-dev] [Code Review] 3576: astobj2: Split hash and rbtree impls into their own source files.

[George Joseph]

> On May 30, 2014, 2:38 p.m., rmudgett wrote:
> > branches/12/main/astobj2_private.h, lines 156-159
> > <https://reviewboard.asterisk.org/r/3576/diff/1/?file=59035#file59035line156>
> >
> >     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.

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?


- George


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


On May 29, 2014, 6:53 p.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3576/
> -----------------------------------------------------------
> 
> (Updated May 29, 2014, 6:53 p.m.)
> 
> 
> Review request for Asterisk Developers and rmudgett.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> 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.  
> 
> 
> Diffs
> -----
> 
>   branches/12/main/astobj2_rbtree.c PRE-CREATION 
>   branches/12/main/astobj2_private.h PRE-CREATION 
>   branches/12/main/astobj2_hash.c PRE-CREATION 
>   branches/12/main/astobj2.c 414877 
> 
> Diff: https://reviewboard.asterisk.org/r/3576/diff/
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> George Joseph
> 
>

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