'Re: [asterisk-dev] [Code Review] data api gsoc2009'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       asterisk-dev
Subject:    Re: [asterisk-dev] [Code Review] data api gsoc2009
From:       "Russell Bryant" <russell () digium ! com>
Date:       2009-06-29 22:42:06
Message-ID: 20090629224206.2484.50948 () hotblack ! digium ! internal
[Download RAW message or body]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/275/#review907
-----------------------------------------------------------

/trunk/include/asterisk/data.h
<http://reviewboard.digium.com/r/275/#comment2173>

    I would have expected the put() callback to receive some kind of argument that \
specifies what field is being set.  Is the design of this part of it still in \
progress?

/trunk/include/asterisk/data.h
<http://reviewboard.digium.com/r/275/#comment2174>

    I think both of these structures should have version fields to be able to detect \
ABI changes in the core.  
    For an example of what I mean, see the security event code that I have up on \
reviewboard right now.  I use version fields for all of the structures defined as \
part of the API in security_event_defs.h.

/trunk/include/asterisk/data.h
<http://reviewboard.digium.com/r/275/#comment2172>

    What is the value of a string registrar argument?  Is it for debugging purposes?

    I think it would be a good idea to have an optional ast_module argument, though.  \
That way, you can automatically handle updating the module reference count when you \
call a callback in a module.

/trunk/include/asterisk/data.h
<http://reviewboard.digium.com/r/275/#comment2175>

    I think this function could use some additional documentation to describe when \
you would want to use this over the normal ast_data_get() API call.  Is it just for \
debugging?

/trunk/include/asterisk/xml.h
<http://reviewboard.digium.com/r/275/#comment2176>

    A picky comment, but can you make the formatting of each block consistent?

    I would prefer \brief on the 2nd line, and each item only 1 space from the '*'.

/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2177>

    Would it make sense to declare these as int32_t and uint32_t to be more explicit \
about the data type?

/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2178>

    Will node order ever be important?  If so, the current astobj2 may not fit the \
bill, since you have no control over ordering amongst the items in the container.

/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2180>

    I would add a note that this gets initialized in ast_data_init(), or someone may \
come around and replace this with AST_RWLOCK_DEFINE_STATIC().  
    I actually prefer not using constructor based initialization for globals, though, \
so I think this way is better.

/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2181>

    There is a small bug here.

    If you want to make the comparison not case sensitive, then you should use \
ast_str_case_hash() as the hash function.  Otherwise, you can run into problems.

/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2182>

    In general, inline functions are better for the sake of type safety.  However, by \
writing it this way, the lock debugging code will always report that the locking \
comes from these functions, instead of where it was _really_ being locked/unlocked.  \
So, I would just use #defines.

/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2183>

    use ast_strlen_zero() here instead.

/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2184>

    Use ast_debug()

/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2185>

    Use ast_strdupa()

/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2186>

    There is no benefit from checking the result of strdupa().  Just assume it was \
successful.

/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2187>

    There is a memory leak here.  You have to unref the container itself.

    Also, once you unref the container, the rest of this code is not necessary.  When \
the container gets destroyed, it will automatically unlink all elements that were \
still in the container.

/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2188>

    I wouldn't put a default case here.  It will force you to reconsider this block \
of code every time you add an entry to the enum if you don't, which is a good thing.

/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2189>

    no need for this check

/trunk/main/data.c
<http://reviewboard.digium.com/r/275/#comment2191>

    I think it would be handy to make the test code a CLI command.  In fact, I think \
it would be a great idea to build up some test code in a test module for the tests \
directory, instead of here.

- Russell

On 2009-06-26 19:19:30, Eliel Sardañons wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/275/
> -----------------------------------------------------------
> 
> (Updated 2009-06-26 19:19:30)
> 
> 
> Review request for Asterisk Developers, Russell Bryant and Tilghman Lesher.
> 
> 
> Summary
> -------
> 
> This is the first review request for the Data API GSoC 2009 project.
> An architectural review is requested.
> 
> 
> Diffs
> -----
> 
> /trunk/include/asterisk/_private.h 203905 
> /trunk/include/asterisk/data.h PRE-CREATION 
> /trunk/include/asterisk/xml.h 203905 
> /trunk/main/asterisk.c 203905 
> /trunk/main/data.c PRE-CREATION 
> /trunk/main/xml.c 203905 
> 
> Diff: http://reviewboard.digium.com/r/275/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Eliel
> 
> 

_______________________________________________
--Bandwidth and Colocation Provided by http://www.api-digital.com--

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

[prev in list] [next in list] [prev in thread] [next in thread] 