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

[rmudgett]

> On Nov. 15, 2012, 3:01 p.m., Mark Michelson wrote:
> > /trunk/main/astobj2.c, lines 5478-5479
> > <https://reviewboard.asterisk.org/r/2110/diff/2/?file=31697#file31697line5478>
> >
> >     Braces, please.

Wasn't part of the change but done.


> On Nov. 15, 2012, 3:01 p.m., Mark Michelson wrote:
> > /trunk/main/astobj2.c, lines 3654-3658
> > <https://reviewboard.asterisk.org/r/2110/diff/2/?file=31697#file31697line3654>
> >
> >     The comment above the if clashes with the return value.

The first comment here is for the whole block of code.  We want to go left.  Since the node has no left child, we are forced to GO_RIGHT.


> On Nov. 15, 2012, 3:01 p.m., Mark Michelson wrote:
> > /trunk/main/astobj2.c, lines 3668-3671
> > <https://reviewboard.asterisk.org/r/2110/diff/2/?file=31697#file31697line3668>
> >
> >     The comment above the if statement clashes with the return.

Similar to previous.


> On Nov. 15, 2012, 3:01 p.m., Mark Michelson wrote:
> > /trunk/main/astobj2.c, line 3648
> > <https://reviewboard.asterisk.org/r/2110/diff/2/?file=31697#file31697line3648>
> >
> >     Can you explain why the object passed into this function is called obj_right? It seems like a misleading name since the direction "right" doesn't necessarily play into things.

It is trying to indicate that the object is to be passed as the right object to the sort_fn.  It is documented identically as the ao2_sort_fn.  That object points to different things depending upon what the flags value indicates: OBJ_POINTER, OBJ_KEY, OBJ_PARTIAL_KEY.


> On Nov. 15, 2012, 3:01 p.m., Mark Michelson wrote:
> > /trunk/main/astobj2.c, lines 1116-1118
> > <https://reviewboard.asterisk.org/r/2110/diff/2/?file=31697#file31697line1116>
> >
> >     I recommend adding a comment inside chan_iax2 to the specific places where the hash function is abused that explains why the use is bad and possibly suggests fixes.

Added a comment to chan_iax2.


> On Nov. 15, 2012, 3:01 p.m., Mark Michelson wrote:
> > /trunk/tests/test_astobj2.c, line 457
> > <https://reviewboard.asterisk.org/r/2110/diff/2/?file=31700#file31700line457>
> >
> >     I get a compile warning (gcc 4.4) on this line. "'n_buckets' may be used uninitialized in this function"
> >     
> >     Personally, I don't see it. All possible values for test_container_type are handled in the switch statement, and each of those is guaranteed to set n_buckets to something.
> >     
> >     I thought it was weird I was seeing it here but not in trunk. Turns out I was never compiling this file in trunk when I'd test-compile things. Sure enough, I get the same warning there, too.
> >     
> >     So, while I think the compiler is full of it, you'll need to do something to get rid of the warning. I got around it by adding a default case to the switch and returning a test failure.

The compiler is complaining because the value passed in as type may not have a defined enum value.

Matt already took care of this warning for me. :)


- rmudgett


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


On Oct. 2, 2012, 6:52 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2110/
> -----------------------------------------------------------
> 
> (Updated Oct. 2, 2012, 6:52 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 374281 
>   /trunk/include/asterisk/test.h 374281 
>   /trunk/main/astobj2.c 374281 
>   /trunk/main/channel.c 374281 
>   /trunk/main/test.c 374281 
>   /trunk/tests/test_astobj2.c 374281 
> 
> 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/20121120/7ed6ddb8/attachment-0001.htm>