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

[rmudgett]

> On May 30, 2014, 3: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.
> 
> George Joseph wrote:
>     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?
>

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.


- rmudgett


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


On May 29, 2014, 7: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, 7: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/20140531/75e4dd6a/attachment.html>