Re: [PATCH v2 02/13] tools/xenstore: do some cleanup of hashtable.c

[Juergen Gross <jgross@xxxxxxxx>]

On 03.05.23 14:55, Julien Grall wrote:
> Hi Juergen,
>
> On 30/03/2023 09:50, Juergen Gross wrote:
> > Do the following cleanups:
> > - hashtable_count() isn't used at all, so remove it
> > - replace prime_table_length and max_load_factor with macros
> > - make hash() static
> > - add a loadlimit() helper function
> > - remove the /***/ lines between functions
> > - do some style corrections
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> >   tools/xenstore/hashtable.c | 71 ++++++++++++++------------------------
> >   tools/xenstore/hashtable.h | 10 ------
> >   2 files changed, 26 insertions(+), 55 deletions(-)
> >
> > diff --git a/tools/xenstore/hashtable.c b/tools/xenstore/hashtable.c
> > index 3d4466b597..c1b11743bb 100644
> > --- a/tools/xenstore/hashtable.c
> > +++ b/tools/xenstore/hashtable.c
> > @@ -40,22 +40,25 @@ static const unsigned int primes[] = {
> >   50331653, 100663319, 201326611, 402653189,
> >   805306457, 1610612741
> >   };
> > -const unsigned int prime_table_length = sizeof(primes)/sizeof(primes[0]);
> > -const unsigned int max_load_factor = 65; /* percentage */
> > -/*****************************************************************************/
> > -/* indexFor */
> > -static inline unsigned int
> > -indexFor(unsigned int tablelength, unsigned int hashvalue) {
> > +#define PRIME_TABLE_LEN   ARRAY_SIZE(primes)
> > +#define MAX_LOAD_PERCENT  65
> > +
> > +static inline unsigned int indexFor(unsigned int tablelength,
> > +                                    unsigned int hashvalue)
> > +{
> >       return (hashvalue % tablelength);
> >   }
> > -/*****************************************************************************/
> > -struct hashtable *
> > -create_hashtable(const void *ctx, unsigned int minsize,
> > -                 unsigned int (*hashf) (const void *),
> > -                 int (*eqf) (const void *, const void *),
> > -                 unsigned int flags)
> > +static unsigned int loadlimit(unsigned int pindex)
> > +{
> > +    return ((uint64_t)primes[pindex] * MAX_LOAD_PERCENT) / 100;
> > +}
> > +
> > +struct hashtable *create_hashtable(const void *ctx, unsigned int minsize,
> > +                                   unsigned int (*hashf) (const void *),
> > +                                   int (*eqf) (const void *, const void *),
> > +                                   unsigned int flags)
> >   {
> >       struct hashtable *h;
> >       unsigned int pindex, size = primes[0];
> > @@ -64,7 +67,7 @@ create_hashtable(const void *ctx, unsigned int minsize,
> >       if (minsize > (1u << 30)) return NULL;
> >       /* Enforce size as prime */
> > -    for (pindex=0; pindex < prime_table_length; pindex++) {
> > +    for (pindex=0; pindex < PRIME_TABLE_LEN; pindex++) {
> As you fix the style, how about adding a space before/after '=' and...
>
> >           if (primes[pindex] > minsize) { size = primes[pindex]; break; }
> ... break this line in multiple ones?
Will do both.

> With or without this included here:
>
> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>
Thanks,


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature