[pacman-dev] [PATCH 1/6] Split optdep into alpm_depend_t and description

[Dan McGee]

On Thu, Jul 21, 2011 at 1:39 PM, Benedikt Morbach
<benedikt.morbach at googlemail.com> wrote:
> This is done as a preparation to better handle optdepends.
> This commit should not change pacman's behaviour, as it simply adds new
> functions and data structures and doesn't yet hook them up anywhere.
> ---
>  lib/libalpm/alpm.h |   12 +++++++++
>  lib/libalpm/deps.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/libalpm/deps.h |    3 ++
>  3 files changed, 85 insertions(+), 0 deletions(-)
>
> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> index 6e1e4bc..21f65a1 100644
> --- a/lib/libalpm/alpm.h
> +++ b/lib/libalpm/alpm.h
> @@ -130,6 +130,12 @@ typedef struct _alpm_depend_t {
>        alpm_depmod_t mod;
>  } alpm_depend_t;
>
> +/** Optional dependency */
> +typedef struct _alpm_optdepend_t {
> +       alpm_depend_t *depend;
Haven't looked through everything yet, but it probably makes more
sense to make this not a pointer- that way you can alloc and free the
optdep structure in one single memory allocation.

> +       char *description;
> +} alpm_optdepend_t;
> +
>  /** Missing dependency */
>  typedef struct _alpm_depmissing_t {
>        char *target;
> @@ -1021,6 +1027,12 @@ alpm_list_t *alpm_checkconflicts(alpm_handle_t *handle, alpm_list_t *pkglist);
>  */
>  char *alpm_dep_compute_string(const alpm_depend_t *dep);
>
> +/** Returns a newly allocated string representing the optional dependency information.
> + * @param dep a optional dependency info structure
> + * @return a formatted string, e.g. "sqlite: for Database support"
> + */
> +char *alpm_optdep_compute_string(const alpm_optdepend_t *optdep);
> +
>  /** @} */
>
>  /** @} */
> diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c
> index c3681b3..2f4f53a 100644
> --- a/lib/libalpm/deps.c
> +++ b/lib/libalpm/deps.c
> @@ -44,6 +44,13 @@ void _alpm_dep_free(alpm_depend_t *dep)
>        FREE(dep);
>  }
>
> +void _alpm_optdep_free(alpm_optdepend_t *optdep)
> +{
> +       _alpm_dep_free(optdep->depend);
Of course if you take my suggestion above, you won't be able to do
this anymore- you will just want to do
    FREE(optdep->dep.name);
    FREE(optdep->dep.version);
here.

> +       FREE(optdep->description);
> +       FREE(optdep);
> +}
> +
>  static alpm_depmissing_t *depmiss_new(const char *target, alpm_depend_t *dep,
>                const char *causingpkg)
>  {
> @@ -466,6 +473,44 @@ alpm_depend_t *_alpm_dep_dup(const alpm_depend_t *dep)
>        return newdep;
>  }
>
> +alpm_optdepend_t *_alpm_splitoptdep(const char *optstring)
> +{
> +       alpm_optdepend_t *optdep;
> +       char *depstring;
> +       const char *descstring;
> +
> +       ASSERT(optstring != NULL, return NULL);
> +
> +       CALLOC(optdep, 1, sizeof(alpm_optdepend_t), return NULL);
> +
> +       if ((descstring = strstr(optstring, ":")) == NULL) {
> +               descstring = optstring + strlen(optstring);
> +       }
I don't like this- it differs from how we do things in splitdep(),
which is to leave version NULL if we didn't have one. We should do the
same with the description field here.

> +
> +       STRNDUP(depstring, optstring, descstring - optstring, return NULL);
> +       optdep->depend = _alpm_splitdep(depstring);
Err...now I see what is going on. It seems to be it would be better in
some fashion if splitdep() just handled (and ignored?) the version
part, or something along those lines.

I also see another problem here- now that we have epoch and it uses a
':' separator, this simple logic here is going to fall apart.

> +       FREE(depstring);
> +
> +       if (*descstring == '\0') {
> +               optdep->description = NULL;
calloc already nulled this out, so you don't need to do it again.

> +       } else {
> +               STRDUP(optdep->description, descstring + 1, return NULL);
> +       }
> +
> +       return optdep;
> +}
> +
> +alpm_optdepend_t *_alpm_optdep_dup(const alpm_optdepend_t *optdep)
> +{
> +       alpm_optdepend_t *newdep;
> +       CALLOC(newdep, 1, sizeof(alpm_optdepend_t), return NULL);
> +
> +       newdep->depend = _alpm_dep_dup(optdep->depend);
> +       STRDUP(newdep->description, optdep->description, return NULL);
> +
> +       return newdep;
> +}
> +
>  /* These parameters are messy. We check if this package, given a list of
>  * targets and a db is safe to remove. We do NOT remove it if it is in the
>  * target list, or if if the package was explictly installed and
> @@ -820,4 +865,29 @@ char SYMEXPORT *alpm_dep_compute_string(const alpm_depend_t *dep)
>
>        return str;
>  }
> +
> +/** Reverse of splitoptdep; make a optdep string from a alpm_optdepend_t struct.
> + * The string must be freed!
> + * @param optdep the optdepend to turn into a string
> + * @return a string-formatted optional dependency with description
> + */
> +char SYMEXPORT *alpm_optdep_compute_string(const alpm_optdepend_t *optdep)
> +{
> +       char *str, *depstring;
> +       size_t len;
> +
> +       ASSERT(optdep       != NULL, return NULL);
What's going on here with all the extra spaces?

> +
> +       depstring = alpm_dep_compute_string(optdep->depend);
> +
> +       if(optdep->description != NULL) {
> +               len = strlen(depstring) + strlen(optdep->description) + 2;
> +               MALLOC(str, len, return NULL);
> +               snprintf(str, len, "%s:%s", depstring, optdep->description);
Don't we usually print a single space after the colon? I thought that
was our established format.
> +               FREE(depstring);
> +               return str;
> +       } else {
> +               return depstring;
> +       }
This is nitpicking a bit, but a few things here. str can be defined
inside the if block since it is only used there. The else is not
strictly necessary as you returned from within the if anyway. Finally,
you can use free() instead of FREE() (although the complier should be
smart enough anyway to optimize away the unnecessary NULL-ing).

And it looks best/easiest to just ignore my struct in struct comment
at the start of this- so many of our dep functions are allocating the
object already that is isn't worth changing all of them around right
now to accommodate a slightly smaller memory footprint.

> +}
>  /* vim: set ts=2 sw=2 noet: */
> diff --git a/lib/libalpm/deps.h b/lib/libalpm/deps.h
> index 6ef4cbb..acdd25a 100644
> --- a/lib/libalpm/deps.h
> +++ b/lib/libalpm/deps.h
> @@ -28,7 +28,9 @@
>  #include "alpm.h"
>
>  void _alpm_dep_free(alpm_depend_t *dep);
> +void _alpm_optdep_free(alpm_optdepend_t *optdep);
>  alpm_depend_t *_alpm_dep_dup(const alpm_depend_t *dep);
> +alpm_optdepend_t *_alpm_optdep_dup(const alpm_optdepend_t *optdep);
>  void _alpm_depmiss_free(alpm_depmissing_t *miss);
>  alpm_list_t *_alpm_sortbydeps(alpm_handle_t *handle, alpm_list_t *targets, int reverse);
>  void _alpm_recursedeps(alpm_db_t *db, alpm_list_t *targs, int include_explicit);
> @@ -36,6 +38,7 @@ int _alpm_resolvedeps(alpm_handle_t *handle, alpm_list_t *localpkgs, alpm_pkg_t
>                alpm_list_t *preferred, alpm_list_t **packages, alpm_list_t *remove,
>                alpm_list_t **data);
>  alpm_depend_t *_alpm_splitdep(const char *depstring);
> +alpm_optdepend_t *_alpm_splitoptdep(const char *optstring);
>  int _alpm_depcmp(alpm_pkg_t *pkg, alpm_depend_t *dep);
>
>  #endif /* _ALPM_DEPS_H */
> --
> 1.7.6