[asterisk-dev] [Code Review] astobj2: Avoid using temporary objects + ao2_find() with OBJ_POINTER

[Mark Michelson]

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


I like this idea since it makes ao2_containers operate more like how hashtables and dictionaries typically do. That is, you look up an object by its key, not by using another object of the same type. I initially was going to suggest moving towards the entire eradication of OBJ_POINTER, but since some structures may have more complicated hashes than just a single field (and they typically don't store their location information in a sub-struct), this may not be the best idea. The name OBJ_CUSTOM makes sense, but I think it could be improved. Perhaps something like OBJ_KEY or OBJ_HASHKEY would fit, since you are looking up an object by some key value.


/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/1184/#comment6983>

    I'd add a note here stating that OBJ_CUSTOM and OBJ_POINTER are mutually exclusive options.



/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/1184/#comment6984>

    Somewhere before here, you should add an ast_assert() to be sure that this hasn't been called with both OBJ_POINTER and OBJ_CUSTOM set.



/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/1184/#comment6985>

    I'm not sure why you would add the const qualifier just to immediately cast to non-const. The better idea here would be to change to const across the board since the arg should be const, but I know that would mean touching a LOT of files. For now, just revert to taking a non-const.
    
    EDIT: Okay, I see what's up now. You did this because changing to using OBJ_CUSTOM resulted in passing const structures to ao2_find and ao2_callback, and this was necessary in order to compile. I still think the long-term fix of adding const all over is better, but I understand that would be a lot of work for an idea that may get rejected by your reviewers :)


- Mark


On 2011-04-15 14:02:18, Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1184/
> -----------------------------------------------------------
> 
> (Updated 2011-04-15 14:02:18)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> There is a fairly common pattern making its way through the code base where we put a temporary object on the stack so we can call ao2_find() with OBJ_POINTER.  The purpose is so that it can be passed into the object hash function.  However, this really seems like a hack and potentially error prone.  This patch is a first stab at approach to avoid having to do that.
> 
> It adds a new flag, OBJ_CUSTOM, which can be used instead of OBJ_POINTER in these situations.  Then, the hash function can know whether it was given an object or some custom data to hash.
> 
> The patch also changes some uses of ao2_find() for iax2_user and iax2_peer objects to reflect how OBJ_CUSTOM would be used.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_iax2.c 313901 
>   /trunk/include/asterisk/astobj2.h 313901 
>   /trunk/main/astobj2.c 313901 
> 
> Diff: https://reviewboard.asterisk.org/r/1184/diff
> 
> 
> Testing
> -------
> 
> It compiles.  I'm just throwing out the idea right now.  If people seem to think it makes sense, I will certainly try calls before committing anything.
> 
> 
> Thanks,
> 
> Russell
> 
>

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