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

[Russell Bryant]

> On 2011-04-15 23:10:32, Mark Michelson wrote:
> > 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.

Thanks a lot for the review, Mark!  I like your name suggestion.  I'll change it in the next rev.


> On 2011-04-15 23:10:32, Mark Michelson wrote:
> > /trunk/include/asterisk/astobj2.h, lines 683-690
> > <https://reviewboard.asterisk.org/r/1184/diff/1/?file=16171#file16171line683>
> >
> >     I'd add a note here stating that OBJ_CUSTOM and OBJ_POINTER are mutually exclusive options.

Will do


> On 2011-04-15 23:10:32, Mark Michelson wrote:
> > /trunk/main/astobj2.c, lines 646-652
> > <https://reviewboard.asterisk.org/r/1184/diff/1/?file=16172#file16172line646>
> >
> >     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.

good idea, will do


> On 2011-04-15 23:10:32, Mark Michelson wrote:
> > /trunk/main/astobj2.c, lines 804-812
> > <https://reviewboard.asterisk.org/r/1184/diff/1/?file=16172#file16172line804>
> >
> >     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 :)

Yep, in your EDIT you described exactly why I did this.  If you add const to ao2_callback(), the changes get much more invasive since it affects all ao2_callback() users (and there are a bunch of them).  Also, the only place arg ends up being used is to pass it in to the hash function, and its argument is const.


- Russell


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


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