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

[rmudgett]

> On Oct. 1, 2012, 1:45 p.m., Mark Michelson wrote:
> > 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.

1) My editor thinks print() is a library function and highlights it that way.  Also print() would be very close to printf().  All the other uses inside longer identifiers is just consistency.

2) That can be done in another patch.  There are going to be some scoping issues breaking this up.  ao2_container_clone() looks into the ao2 object to get configuration information.  The RTTI code also slightly violates scoping.


> On Oct. 1, 2012, 1:45 p.m., Mark Michelson wrote:
> > /trunk/include/asterisk/astobj2.h, line 940
> > <https://reviewboard.asterisk.org/r/2110/diff/1/?file=31203#file31203line940>
> >
> >     s/childeren/children/

done


> On Oct. 1, 2012, 1:45 p.m., Mark Michelson wrote:
> > /trunk/include/asterisk/astobj2.h, line 949
> > <https://reviewboard.asterisk.org/r/2110/diff/1/?file=31203#file31203line949>
> >
> >     s/Childeren/Children/

done


> On Oct. 1, 2012, 1:45 p.m., Mark Michelson wrote:
> > /trunk/main/astobj2.c, lines 4038-4040
> > <https://reviewboard.asterisk.org/r/2110/diff/1/?file=31205#file31205line4038>
> >
> >     Any particular reason you didn't use SWAP here? I could understand if you reference is_red somewhere else, but you don't.

doomed->is_red is a bit field.  SWAP() does not work with bitfields.


> On Oct. 1, 2012, 1:45 p.m., Mark Michelson wrote:
> > /trunk/main/astobj2.c, lines 3710-3729
> > <https://reviewboard.asterisk.org/r/2110/diff/1/?file=31205#file31205line3710>
> >
> >     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.

done


> On Oct. 1, 2012, 1:45 p.m., Mark Michelson wrote:
> > /trunk/main/astobj2.c, lines 3631-3632
> > <https://reviewboard.asterisk.org/r/2110/diff/1/?file=31205#file31205line3631>
> >
> >     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.

Added enum for GO_LEFT and GO_RIGHT.


- rmudgett


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


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/20121002/3164da7f/attachment-0001.htm>