Thanks for the patch. What did you wrap the comments to? We normally wrap the doxygen to about 80 columns just like the code. I have concerns about some of the specific documentation explained below.
On 08/14/13 at 03:10pm, Richard Pougnet wrote: > Signed-off-by: Richard Pougnet <[email protected]> > --- > lib/libalpm/add.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 49 insertions(+), 3 deletions(-) > > diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c > index 45c57b0..73112fc 100644 > --- a/lib/libalpm/add.c > +++ b/lib/libalpm/add.c > @@ -47,7 +47,16 @@ > #include "remove.h" > #include "handle.h" > > -/** Add a package to the transaction. */ > +/** Adds a package to transaction add list > + * Finds the target package and compares > + * to the local version (if it exists). > + * If version is newer then cache, or downgrade > + * is desired, the package is then added to the list > + * and log file is updated. > + * @param handle the context handle > + * @param pkg the package to be added > + * @return 0 on success, 1 on error > + */ > int SYMEXPORT alpm_add_pkg(alpm_handle_t *handle, alpm_pkg_t *pkg) > { > const char *pkgname, *pkgver; This already has full doxygen documentation in alpm.h. Unfortunately, we have several functions like this with real documentation in alpm.h and a stub at the function definition. > @@ -104,6 +113,18 @@ int SYMEXPORT alpm_add_pkg(alpm_handle_t *handle, > alpm_pkg_t *pkg) > return 0; > } > > +/** Extracts a package. > + * Loads the package archive, reads it > + * and then extracts. > + * If non critical error occured, logs and > + * continues. Else, extract fails. > + * @param handle the context handle > + * @param archive the package archive > + * @param entry the entry to be extracted > + * @param filename the filename of the archive > + * @param origname the name of the package > + * @return 0 on success, 1 on error > + */ > static int perform_extraction(alpm_handle_t *handle, struct archive *archive, > struct archive_entry *entry, const char *filename, const char > *origname) > { This neither loads nor extracts a *package*. It extracts a single file from an already loaded package. Filename and origname aren't the name of the archive or package; they're the path to the file being extracted. > @@ -129,6 +150,12 @@ static int perform_extraction(alpm_handle_t *handle, > struct archive *archive, > } > return 0; > } There should be a blank line here. > +/** Attempts to rename an entry > + * @param handle the context handler > + * @param src the original name > + * @param dest the final name > + * @return 0 on success, 1 on error > +*/ > > static int try_rename(alpm_handle_t *handle, const char *src, const char > *dest) > { > @@ -141,7 +168,14 @@ static int try_rename(alpm_handle_t *handle, const char > *src, const char *dest) > } > return 0; > } > - > +/** Extracts a single file > + * @param handle the context handler > + * @param archive the package archive > + * @param entry the archive entry > + * @param newpkg the new package > + * @param oldpkg the old package > + * @return 0 on success, 1 on error > +*/ > static int extract_single_file(alpm_handle_t *handle, struct archive > *archive, > struct archive_entry *entry, alpm_pkg_t *newpkg, alpm_pkg_t > *oldpkg) > { The overwhelming bulk of this function is about the proper handling of backup files. The description should acknowledge that fact. > @@ -450,7 +484,15 @@ needbackup_cleanup: > free(entryname_orig); > return errors; > } > - Why did you remove the blank lines? > +/** Commits a package to the database > + * Creates a database entry for a newly installed > + * package. > + * @param handle the context handler > + * @param newpkg the package to be entered > + * @param pkg_current the current package number > + * @param pkg_count total number of packages to be committed > + * @return 0 on success, -1 on error > +*/ > static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, > size_t pkg_current, size_t pkg_count) > { This doesn't just create the database entry, it's responsible for the actual installation of the package as well. > @@ -667,6 +709,10 @@ cleanup: > return ret; > } > > +/** Upgrades a package > + * @param handle the context handler > + * @return 0 on success, -1 on error > +*/ > int _alpm_upgrade_packages(alpm_handle_t *handle) > { > size_t pkg_count, pkg_current; This doesn't just "upgrade a package". This function is responsible for the entire actual installation process of all of the packages being installed. > -- > 1.8.3.4
