eiSList

[kendrick, 2010-11-04 04:27:05]

My 2 cents opinion.

http://codereview.appspot.com/2862041/diff/1/eiCORE/UnitTest/eiSListUnitTest.h
File eiCORE/UnitTest/eiSListUnitTest.h (right):

http://codereview.appspot.com/2862041/diff/1/eiCORE/UnitTest/eiSListUnitTest....
eiCORE/UnitTest/eiSListUnitTest.h:21: typedef struct myNode
I am not sure how the list is designed. Looks like it is an analogy to c++'s
inheritance, where the base class is ei_sList_node? How to later access the data
given the list? The pointer of the data is at an offset of
sizeof(ei_sList_node)? Would it be a good design idea?

http://codereview.appspot.com/2862041/diff/1/eiCORE/UnitTest/eiSListUnitTest....
eiCORE/UnitTest/eiSListUnitTest.h:46: ei_slist* slist =
(ei_slist*)ei_allocate(sizeof(ei_slist));
mem leak?

http://codereview.appspot.com/2862041/diff/1/eiCORE/eiSList.c
File eiCORE/eiSList.c (right):

http://codereview.appspot.com/2862041/diff/1/eiCORE/eiSList.c#newcode53
eiCORE/eiSList.c:53: if (slist->tail == slist->tail )
What is the deal here? Shouldn't tail always be set?

http://codereview.appspot.com/2862041/diff/1/eiCORE/eiSList.c#newcode100
eiCORE/eiSList.c:100: eiBool ei_slist_clear(ei_slist* slist)
Again, I am not quite sure the whole single list design. Would you consider to
have a callback registered in order to release node's memory when clear?
Otherwise, you have to keep explicit record of all your nodes and release later.
Is this the plan? Or you are planning to implement garbage collection?

http://codereview.appspot.com/2862041/diff/1/eiCORE/eiSList.c#newcode120
eiCORE/eiSList.c:120: p = NULL;
Is this useless, no?

http://codereview.appspot.com/2862041/diff/1/eiCORE/eiSList.h
File eiCORE/eiSList.h (right):

http://codereview.appspot.com/2862041/diff/1/eiCORE/eiSList.h#newcode32
eiCORE/eiSList.h:32: 
In general, what is our naming convention here? Seems we ditched eiAppleBanana
kind of naming, but adopted ei_apple_banana? Then it is little bit confusing to
see eiSizet at the same time. Wouldn't it be ei_size_t then?

http://codereview.appspot.com/2862041/diff/1/eiCORE/eiSList.h#newcode49
eiCORE/eiSList.h:49: EI_API_EXPORT eiSizet ei_slist_size(const ei_slist* slist);
Do we consider "ei_slist const *" to better differentiate between two const
usages? Like this emphasizes the pointer to be const rather than "ei_slist *
const" which emphasizes the referenced var is constant.

[Elvic, 2010-11-04 07:13:57]

Generally good to me, Kaichi's comments on the algorithm is remarkable, I would
take a deeper look later.

http://codereview.appspot.com/2862041/diff/1/eiCORE/eiSList.c
File eiCORE/eiSList.c (right):

http://codereview.appspot.com/2862041/diff/1/eiCORE/eiSList.c#newcode100
eiCORE/eiSList.c:100: eiBool ei_slist_clear(ei_slist* slist)
On 2010/11/04 04:27:05, kendrick wrote:

The design is to be "intrusive", as you said, users are supposed to allocate and
release the memory on their own. This design gives us more flexibility on memory
management, with the cost of reducing usability.

These functions are only meant to manipulate "linkages", no other sorts of
operations should be performed.

http://codereview.appspot.com/2862041/diff/1/eiCORE/eiSList.h
File eiCORE/eiSList.h (right):

http://codereview.appspot.com/2862041/diff/1/eiCORE/eiSList.h#newcode32
eiCORE/eiSList.h:32: 
On 2010/11/04 04:27:05, kendrick wrote:

I guess this is the special naming convention for our STL-analogue containers.

http://codereview.appspot.com/2862041/diff/1/eiCORE/eiSList.h#newcode49
eiCORE/eiSList.h:49: EI_API_EXPORT eiSizet ei_slist_size(const ei_slist* slist);
On 2010/11/04 04:27:05, kendrick wrote:

Agreed.