[asterisk-dev] [Code Review] RFC for proposed astobj2 API container enhancements

Thu Mar 29 09:50:38 CDT 2012

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


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/1835/#comment10829>

    I don't really like the term "container key item". I think that something like "search key" would be better.


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/1835/#comment10817>

    It's spelled "descending"


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/1835/#comment10818>

    What effect would these flags have on a non-tree? No effect? Emit a warning under dev-mode? Assert under dev-mode?
    
    Defining OBJ_ORDER_POST as you have here is dangerous since if it is set then OBJ_ORDER_DESCENDING and OBJ_ORDER_PRE also evaluate true. You can code around this, but it's too risky for my tastes. You're basically mandating that
    1) All OBJ_ORDER flags have to be evaluated to know for sure which order is being requested.
    2) OBJ_ORDER flags have to be evaluated in a specific order to know for sure which order is being requested.
    
    Just use separate bits for each flag.


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/1835/#comment10819>

    "sense"


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/1835/#comment10821>

    Having a flag evaluate to '0' doesn't work.
    
    Presumably, you're going to have some code that does something like
    
    if (flags & AO2_CONTAINER_ALLOC_OPT_INSERT_END) {
        do_something();
    }
    
    Having a 0-defined flag means that the if will never evaluate true.
    
    My guess here is that your intention is for this to be a "default" behavior, meaning that you will assume this behavior unless a  conflicting option is specified. This is inherently risky though because someone may attempt to actually use this flag in other code, not realizing that a logical and will never evaluate true.
    
    I suggest either
    1) Remove the flag altogether and document default behavior in ao2_container_alloc()
    2) Keep the flag but make it non-zero. You can then document that if an ordering is not specified, then AO2_CONTAINER_ALLOC_OPT_INSERT_END is implied.


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/1835/#comment10820>

    (0 << 1) ?
    
    I'm going to assume again that this flag does not actually get evaluated anywhere and is provided to indicate default behavior unless a conflicting flag is set. My same advice for AO2_CONTAINER_ALLOC_OPT_INSERT_END applies here; either get rid of the flag altogether or make it evaluate to something other than 0.


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/1835/#comment10822>

    Either change this to "duplicate-keyed objects" (added hyphen) or to "objects with duplicate keys". I prefer the second one.


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/1835/#comment10827>

    How do you plan to detect a duplicate object?


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/1835/#comment10823>

    This flag definition is haphazard because if someone sets AO2_CONTAINER_ALLOC_OPT_DUPS_REPLACE, then the other two AO2_CONTAINER_ALLOC_OPT_DUPS flags evaluate true as well. Code can be written to handle this properly, but it's just done way too riskily here for my tastes.
    
    Don't be afraid to use more bits. We've got plenty of them :)


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/1835/#comment10824>

    Why is this needed?


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/1835/#comment10825>

    A couple of things:
    
    1) Does providing a sort_fn imply that the container is sorted? In other words, will every object linked into the container undergo sorting so that it is inserted in the proper spot? Based on the fact there appears to be no ao2_sort() function or anything similar, I'm assuming so. Would there be value to providing a sort_fn but not having the container be automatically sorted? I'm thinking of a case where the order of elements in a container typically does not matter but for the output of a CLI or AMI command, we want to list objects in a particular order.
    
    2) What does it mean to sort a hash table? Does the sort_fn only apply to items with duplicate keys (i.e. in the same bucket)? The phrase "buckets not sorted" is ambiguous in your parenthetical note.


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/1835/#comment10828>

    Shouldn't a sort_fn be required for a red-black tree? Also, a red-black tree doesn't have "buckets".


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/1835/#comment10826>

    In a hash table, does this mean that the items within a bucket are traversed in descending order AND that the buckets themselves are traversed in descending order?
    
    Also, again, the word is "descending".
    
    As a final note, you provide preorder and postorder traversal options for ao2_callback(). Shouldn't the same options be available for iterators?


- Mark


On March 28, 2012, 10:51 a.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1835/
> -----------------------------------------------------------
> 
> (Updated March 28, 2012, 10:51 a.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and Matt Jordan.
> 
> 
> Summary
> -------
> 
> RFC to add proposed API enhancements for containers.
> 
> API allows for sorted containers, insertion options, duplicate handling options, and traversal order options.
> 
> Also has several documentation corrections.
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/astobj2.h 360708 
> 
> Diff: https://reviewboard.asterisk.org/r/1835/diff
> 
> 
> Testing
> -------
> 
> It compiles but doesn't link.  This is a RFC.  :)
> 
> 
> Thanks,
> 
> rmudgett
> 
>

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