[pacman-dev] [PATCH] libalpm/remove.c - add doxygen

[Dan McGee]

On Sun, May 20, 2012 at 9:18 PM,  <andrew.gregory.8 at gmail.com> wrote:
> From: Andrew Gregory <andrew.gregory.8 at gmail.com>
>
> Signed-off-by: Andrew Gregory <andrew.gregory.8 at gmail.com>
> ---
>  lib/libalpm/remove.c |  100 +++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 95 insertions(+), 5 deletions(-)
>
> diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c
> index 469ce9b..dfbad00 100644
> --- a/lib/libalpm/remove.c
> +++ b/lib/libalpm/remove.c
> @@ -43,6 +43,15 @@
>  #include "handle.h"
>  #include "conflict.h"
>
> +/**
> + * @brief Add a package removal action to the transaction.
> + *
> + * @param handle the context handle
> + * @param pkg    the package to uninstall
Please don't waste time space-aligning arguments, it just makes for
needless churn when someone adds any strlen(foo) + 1 length argument.
> + *
> + * @return  0: success
> + *         -1: error (pm_errno is set accordingly)
We don't do this format anywhere else, so please don't introduce it
here- see other documentation for examples and ensure it looks good in
generated HTML documentation. There is already a good example on
_alpm_remove_prepare in this very file.

> + */
>  int SYMEXPORT alpm_remove_pkg(alpm_handle_t *handle, alpm_pkg_t *pkg)
>  {
>        const char *pkgname;
> @@ -73,6 +82,15 @@ int SYMEXPORT alpm_remove_pkg(alpm_handle_t *handle, alpm_pkg_t *pkg)
>        return 0;
>  }
>
> +/**
> + * @brief Add dependencies to the removal transaction for cascading.
> + *
> + * @param handle the context handle
> + * @param lp     TODO
> + *
> + * @return  0: on success
> + *         -1: on error
> + */
>  static int remove_prepare_cascade(alpm_handle_t *handle, alpm_list_t *lp)
>  {
>        alpm_trans_t *trans = handle->trans;
> @@ -105,6 +123,12 @@ static int remove_prepare_cascade(alpm_handle_t *handle, alpm_list_t *lp)
>        return 0;
>  }
>
> +/**
> + * @brief Remove needed packages from the removal transaction.
> + *
> + * @param handle the context handle
> + * @param lp     TODO
> + */
>  static void remove_prepare_keep_needed(alpm_handle_t *handle, alpm_list_t *lp)
>  {
>        alpm_trans_t *trans = handle->trans;
> @@ -135,13 +159,18 @@ static void remove_prepare_keep_needed(alpm_handle_t *handle, alpm_list_t *lp)
>        }
>  }
>
> -/** Transaction preparation for remove actions.
> +/**
> + * @brief Transaction preparation for remove actions.
Why the switch away from the existing format? Honestly outside of
alpm_list.c, we barely use @brief format anywhere. Same with addition
of newlines, etc.

> + *
>  * This functions takes a pointer to a alpm_list_t which will be
>  * filled with a list of alpm_depmissing_t* objects representing
>  * the packages blocking the transaction.
> + *
>  * @param handle the context handle
>  * @param data a pointer to an alpm_list_t* to fill
> - * @return 0 on success, -1 on error
> + *
> + * @return  0: on success
> + *         -1: on error
>  */
>  int _alpm_remove_prepare(alpm_handle_t *handle, alpm_list_t **data)
>  {
> @@ -209,6 +238,16 @@ int _alpm_remove_prepare(alpm_handle_t *handle, alpm_list_t **data)
>        return 0;
>  }
>
> +/**
> + * @brief Check if alpm can delete a file.
> + *
> + * @param handle      the context handle
> + * @param file        file to be removed
> + * @param skip_remove list of files that will not be removed
> + *
> + * @return 1: the file can be deleted
> + *         0: the file cannot be deleted
> + */
>  static int can_remove_file(alpm_handle_t *handle, const alpm_file_t *file,
>                alpm_list_t *skip_remove)
>  {
> @@ -235,8 +274,24 @@ static int can_remove_file(alpm_handle_t *handle, const alpm_file_t *file,
>        return 1;
>  }
>
> -/* Helper function for iterating through a package's file and deleting them
> - * Used by _alpm_remove_commit. */
> +/**
> + * @brief Unlink a package file, backing it up if necessary.
> + *
> + * @note Helper function for iterating through a package's file and deleting
> + *       them.
> + * @note Used by _alpm_remove_commit.
> + *
> + * @param handle      the context handle
> + * @param oldpkg      the package being removed
> + * @param newpkg      the package replacing \a oldpkg
> + * @param fileobj     file to remove
> + * @param skip_remove list of files that shouldn't be removed
> + * @param nosave      whether files should be backed up
> + *
> + * @return  0: success
> + *         -1: error unlinking file
> + *          1: file was skipped or did not exist
> + */
>  static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *oldpkg,
>                alpm_pkg_t *newpkg, const alpm_file_t *fileobj, alpm_list_t *skip_remove,
>                int nosave)
> @@ -354,6 +409,20 @@ static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *oldpkg,
>        return 0;
>  }
>
> +/**
> + * @brief Remove a package's files, optionally skipping its replacement's
> + *        files.
> + *
> + * @param handle     the context handle
> + * @param oldpkg     package to remove
> + * @param newpkg     package to replace \a oldpkg (optional)
> + * @param targ_count current index within the transaction (1-based)
> + * @param pkg_count  the number of packages affected by the transaction
> + *
> + * @return  0: success
> + *         -1: some files could not be deleted
> + *         >0: the number of files that were not deleted
> + */
>  static int remove_package_files(alpm_handle_t *handle,
>                alpm_pkg_t *oldpkg, alpm_pkg_t *newpkg,
>                size_t targ_count, size_t pkg_count)
> @@ -433,6 +502,17 @@ static int remove_package_files(alpm_handle_t *handle,
>        return err;
>  }
>
> +/**
> + * @brief Remove a package from the filesystem.
> + *
> + * @param handle     the context handle
> + * @param oldpkg     package to remove
> + * @param newpkg     package to replace \a oldpkg (optional)
> + * @param targ_count current index within the transaction (1-based)
> + * @param pkg_count  the number of packages affected by the transaction
> + *
> + * @return 0
> + */
>  int _alpm_remove_single_package(alpm_handle_t *handle,
>                alpm_pkg_t *oldpkg, alpm_pkg_t *newpkg,
>                size_t targ_count, size_t pkg_count)
> @@ -488,9 +568,18 @@ int _alpm_remove_single_package(alpm_handle_t *handle,
>                                pkgname);
>        }
>
> -       return 0;
> +       return 0; /* TODO: useful return values */
Please don't use trailing comments; we can spare a line above for
things like this.

>  }
>
> +/**
> + * @brief Remove packages in the current transaction.
> + *
> + * @param handle       the context handle
> + * @param run_ldconfig whether to run ld_config after removing the packages
> + *
> + * @return  0: success
> + *         -1: errors occurred while removing files
> + */
>  int _alpm_remove_packages(alpm_handle_t *handle, int run_ldconfig)
>  {
>        alpm_list_t *targ;
> @@ -508,6 +597,7 @@ int _alpm_remove_packages(alpm_handle_t *handle, int run_ldconfig)
>                        return ret;
>                }
>
> +               /* TODO: _alpm_remove_single_package can't actually return -1 */
I know you mean well by putting these comments here, but these are the
kinds of comments that later stick around and become false when
changes are made elsewhere. You already documented above that we need
better return codes in the _alpm_remove_single_package() function
itself; this is good enough.

>                if(_alpm_remove_single_package(handle, pkg, NULL,
>                                        targ_count, pkg_count) == -1) {
>                        handle->pm_errno = ALPM_ERR_TRANS_ABORT;
> --
> 1.7.10.2