[asterisk-dev] [Code Review] Add red-black tree container type to astobj2.

[Mark Michelson]

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


A couple of general points:

1) I mostly overlooked this in the ao2 container improvements, but you have a real love for the word "prnt" for things. Is there any reason you don't just use the entire word "print"?
2) astobj2.c absolutely needs to be broken up. Its size is nowhere near as bad as certain other source files, but it has reached the point where it is doing too many disparate things all in a single file. I think it should be broken into four files:
        a) A file that does basic object allocation, refcount, and locking functions.
        b) A file that contains base container operations that are container-type agnostic.
        c) A file that contains hash table-specific container functions.
        d) A file that contains rbtree-specific container functions.
   This will be much easier to do now than later.


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/2110/#comment13780>

    s/childeren/children/



/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/2110/#comment13781>

    s/Childeren/Children/



/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/2110/#comment13829>

    The retvals here say TRUE and FALSE, but the actual return values are 0 and 1.  What would probably be even better would be constants called LEFT and RIGHT to be more clear. That way, you wouldn't need all the "Go left" and "Go right" comments in this function.



/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/2110/#comment13830>

    The diagram is nice, but you should mention what the letters in the picture stand for:
    
    N = node
    Ch = child
    p = parent
    
    Same goes for the picture of the right rotation down below.



/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/2110/#comment13831>

    Any particular reason you didn't use SWAP here? I could understand if you reference is_red somewhere else, but you don't.


- Mark


On Sept. 12, 2012, 5:02 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2110/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2012, 5:02 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> * Add red-black tree container type.
> 
> * Add CLI command "astobj2 container dump <name>"
> 
> * Added ao2_container_dump() so the container could be dumped by other
> modules for debugging purposes.
> 
> * Changed ao2_container_stats() so it can be used by other modules like
> ao2_container_check() for debugging purposes.
> 
> 
> This addresses bug ASTERISK-19970.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19970
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/astobj2.h 373005 
>   /trunk/include/asterisk/test.h 373005 
>   /trunk/main/astobj2.c 373005 
>   /trunk/main/channel.c 373005 
>   /trunk/main/test.c 373005 
>   /trunk/tests/test_astobj2.c 373005 
> 
> Diff: https://reviewboard.asterisk.org/r/2110/diff
> 
> 
> Testing
> -------
> 
> Updated the unit tests to check red-black tree containers.
> Unit tests pass.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20121001/aba4b290/attachment-0001.htm>