Re: [PATCH] Refactor: make a separate function to add files

[Guillem Jover <guillem@debian.org>]

Hi!

Some comments about format from the other patch apply here as well.

On Sat, 2009-08-29 at 19:59:20 -0400, David Benjamin wrote:
> Here's the second refactoring patch mentioned in "dpkg list-file performance".
> 
> ===
> Refactor: make a separate function to add files
> 
> Added private function _add_file_to_package for inserting a file to a
> package's entries. The function takes a pos_hint to avoid an O(n^2) loop
> when adding to the end of the list. (This is what the original code
> does, so I have mirrored its behavior.)
> 
> Signed-off-by: David Benjamin <davidben@mit.edu>
> ---
>  src/filesdb.c |   77 ++++++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 49 insertions(+), 28 deletions(-)
> 
> diff --git a/src/filesdb.c b/src/filesdb.c
> index a437a40..e672e94 100644
> --- a/src/filesdb.c
> +++ b/src/filesdb.c
> @@ -116,15 +116,59 @@ static void _erase_pkg_file_data(struct pkginfo *pkg) {
>    pkg->clientdata->files = NULL;
>  }
> + /* add a file to pkg. pos_hint is to help find the end of the list
> +  */
> +typedef struct fileinlist **fileinlist_hint;

No typedefs for structs.

> +static fileinlist_hint _add_file_to_package(struct pkginfo *pkg, const char *file, enum fnnflags flags, fileinlist_hint pos_hint) {

Renamed this to pkg_files_add_file. Renamed pos_hint to the more
descriptive file_tail.

> +  struct fileinlist *newent;
> +  struct filepackages *packageslump;
> +  int putat;

Initialized putat here.

> +  ensure_package_clientdata(pkg);

Added blank line here.

> +  if (pos_hint == NULL)
> +    pos_hint = &pkg->clientdata->files;
> +  /* Make sure we're at the end */
> +  while ((*pos_hint) != NULL) {
> +    pos_hint = &((*pos_hint)->next);
> +  }
> +
> +  /* Create a new node */
> +  newent = nfmalloc(sizeof(struct fileinlist));
> +  newent->namenode = findnamenode(file, flags);
> +  newent->next = NULL;
> +  *pos_hint = newent;
> +  pos_hint = &newent->next;
> +
> +  /* Add pkg to newent's package list */
> +  packageslump = newent->namenode->packages;
> +  putat = 0;

Hmmm and just noticed I forgot to remove this one.

> +  if (packageslump) {
> +    for (; putat < PERFILEPACKAGESLUMP && packageslump->pkgs[putat];
> +         putat++);

Changed this to a while().

> +    if (putat >= PERFILEPACKAGESLUMP)
> +      packageslump = NULL;
> +  }
> +  if (!packageslump) {
> +    packageslump = nfmalloc(sizeof(struct filepackages));
> +    packageslump->more = newent->namenode->packages;
> +    newent->namenode->packages = packageslump;
> +    putat= 0;
> +  }
> +  packageslump->pkgs[putat]= pkg;

Space missing before equal.

> +  if (++putat < PERFILEPACKAGESLUMP)
> +    packageslump->pkgs[putat] = NULL;
> +
> +  /* Return the position for the next guy */
> +  return pos_hint;
> +}
> +

Pushed, thanks!

regards,
guillem