[Freeipa-devel] [PATCH] add dynamic hash table data structure implementation

[Simo Sorce]

On Thu, 2009-04-16 at 23:17 -0400, John Dennis wrote:
> Simo Sorce wrote: 
> > On Thu, 2009-04-16 at 18:14 -0400, John Dennis wrote:
> >   
> > > This adds a dynamic hash table data structure to our common code area.
> > >     
> > 
> > Still reading the patch, but one thing jumped out immediately.
> > Why are you defining TRUE and FALSE and declaring functions as int ?
> >   
> Most of the functions are declared as returning int because they
> return integer error codes, not booleans. There is only one public
> function returning a boolean. However there are several internal
> functions returning a logical boolean value as an int. Each of these
> could be modified to return a bool instead.

Yes I was thinking about these internal functions.
also please use 'return true;' not 'return(TRUE);', I am not sure we
have anything about 'return'  in the style guide, but nowhere else we
use return().

> > Why don't you use bool, true and false ?
> >   
> Good catch, I should use bool. I guess that's what happens when you've
> already been programming in a language for years before a feature is
> added, I still think in terms of zero and non-zero when programming in
> C :-)

Eh, bool is standard since C99 :-)

Btw, even tho I haven't finished reviewing the patch I have another
nitpick: in lookup() you assert() if table is NULL. I really don't like
to see assert() used in libraries. Given in all paths I could spot the
table pointer comes from the calling app I would rather check for it's
validity in the public functions and return EINVAL instead.
Let the app decide if it wants to abort() in that case or handle the
issue differently.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York