[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/#review7392
-----------------------------------------------------------


Nothing but minor things. As far as functionality is concerned, it all seems to be there. In addition, the test cases are very nicely varied, so I couldn't find much that wasn't tested in rb trees.


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

    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.



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

    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.



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

    The comment above the if clashes with the return value.



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

    The comment above the if statement clashes with the return.



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

    Braces, please.



/trunk/tests/test_astobj2.c
<https://reviewboard.asterisk.org/r/2110/#comment14180>

    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.


- Mark


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/20121115/d32582e6/attachment-0001.htm>