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

[rmudgett]

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



branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3576/#comment22040>

    If you put this where the original definition was you can see what was changed easier.



branches/12/main/astobj2.c
<https://reviewboard.asterisk.org/r/3576/#comment22038>

    Move this to before handle_astobj2_stats where it was before to reduce the diff since this didn't need to move.



branches/12/main/astobj2_container.c
<https://reviewboard.asterisk.org/r/3576/#comment22042>

    If you put compile time conditionals around functions and other global items it is easier to know when code is included.  You then don't have to search back possibly hundreds of lines to find if it is even code you are interested in.



branches/12/main/astobj2_container.c
<https://reviewboard.asterisk.org/r/3576/#comment22029>

    This should be static.



branches/12/main/astobj2_container.c
<https://reviewboard.asterisk.org/r/3576/#comment22028>

    This is done by the astobj2 initialization code.



branches/12/main/astobj2_container.c
<https://reviewboard.asterisk.org/r/3576/#comment22030>

    Hmm.  All of this code is conditional on AST_DEVMODE now.



branches/12/main/astobj2_container_private.h
<https://reviewboard.asterisk.org/r/3576/#comment22048>

    Remove as the header itself does not depend on this file.



branches/12/main/astobj2_container_private.h
<https://reviewboard.asterisk.org/r/3576/#comment22043>

    This doesn't need to be known outside of astobj2_container.c.  There was an AST_DEVMODE block in the pre-split astobj2 file that contained these declarations and the reg_containers declaration that should have been moved with the generic container code directly to astobj2_containers.c.
    
    I would prefer that it moved to right before ao2_reg_sort_cb() to limit the scope of the declarations.  This is the same relative position it was in the pre-split astobj2 file.  The reg_containers definition should also be put in this block as well.  However, since this block is only declarations you could put it near the top of the file.



branches/12/main/astobj2_container_private.h
<https://reviewboard.asterisk.org/r/3576/#comment22044>

    Make static.



branches/12/main/astobj2_hash.c
<https://reviewboard.asterisk.org/r/3576/#comment22051>

    Include utils.h here since the implementation file depends on it.



branches/12/main/astobj2_private.h
<https://reviewboard.asterisk.org/r/3576/#comment22047>

    Remove as the header itself does not depend on this file.



branches/12/main/astobj2_rbtree.c
<https://reviewboard.asterisk.org/r/3576/#comment22050>

    Include utils.h here since the implementation file depends on it.



branches/12/tests/test_astobj2.c
<https://reviewboard.asterisk.org/r/3576/#comment22026>

    It is best to only declare one variable per line in case one is deleted you don't need to mess with the other declarations or if you need to make some conditionally compiled.



branches/12/tests/test_astobj2.c
<https://reviewboard.asterisk.org/r/3576/#comment22027>

    ao2_cleanup() is NULL safe so you don't need to test for NULL before calling.  That was the point of the change from ao2_ref() to ao2_cleanup().


- rmudgett


On June 4, 2014, 8:33 p.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3576/
> -----------------------------------------------------------
> 
> (Updated June 4, 2014, 8:33 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.  
> 
> EDIT:..
> Also added a basic test to the test framework to monitor performance impacts as changes are made to astobj2.
> 
> 
> Diffs
> -----
> 
>   branches/12/utils/Makefile 415187 
>   branches/12/tests/test_astobj2.c 415187 
>   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_container_private.h PRE-CREATION 
>   branches/12/main/astobj2_container.c PRE-CREATION 
>   branches/12/main/astobj2.c 415187 
>   branches/12/include/asterisk/astobj2.h 415187 
> 
> 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/20140605/5cfaf1e8/attachment-0001.html>