[pacman-dev] Various code-cleanups
* Removed some unnecessary headers and library links * Made things static if possible * Cleaned up makefiles a bit * Fixed some old comments in the code * Fixed some errors the static code checker splint pointed out (unnecessary = NULL, bad exit() syntax, casting problems, etc.) * Backwards arguments in a memset call in _alpm_db_read (could have been worse) * Other various small fixes Signed-off-by: Dan McGee <dpmcgee@gmail.com> (also posted at http://pastebin.archlinux.org/878 to make line wraps not a concern) diff -Naurp pacman-lib.orig/bindings/perl/Makefile.in pacman-lib.codecleanup/bindings/perl/Makefile.in --- pacman-lib.orig/bindings/perl/Makefile.in 2006-10-25 11:40:44.000000000 -0400 +++ pacman-lib.codecleanup/bindings/perl/Makefile.in 2007-01-17 10:41:18.000000000 -0500 @@ -34,7 +34,7 @@ install-pm: Core.pm install -m644 $^ $(DESTDIR)$(LIBDIR)/Alpm/ clean: - rm -f Core* alpm{.h,_wrap*} + rm -f Core* alpm{.h,.i,_wrap*} distclean: clean rm -f Makefile diff -Naurp pacman-lib.orig/bindings/python/Makefile.in pacman-lib.codecleanup/bindings/python/Makefile.in --- pacman-lib.orig/bindings/python/Makefile.in 2006-12-14 00:23:08.000000000 -0500 +++ pacman-lib.codecleanup/bindings/python/Makefile.in 2007-01-17 10:42:02.000000000 -0500 @@ -41,7 +41,7 @@ install-py: alpm.py alpm.pyc install -m644 $^ $(DESTDIR)$(LIBDIR) clean: - rm -f _alpm* alpm{.h,.py*,_wrap*} + rm -f _alpm* alpm{.h,.i,.py*,_wrap*} distclean: clean rm -f Makefile diff -Naurp pacman-lib.orig/lib/libalpm/Makefile.am pacman-lib.codecleanup/lib/libalpm/Makefile.am --- pacman-lib.orig/lib/libalpm/Makefile.am 2006-12-14 00:23:08.000000000 -0500 +++ pacman-lib.codecleanup/lib/libalpm/Makefile.am 2007-01-17 10:43:23.000000000 -0500 @@ -40,7 +40,7 @@ include_HEADERS = alpm.h libalpm_la_SOURCES = $(TARGETS) libalpm_la_LDFLAGS = -no-undefined -version-info $(PM_VERSION_INFO) -libalpm_la_LIBADD = -ldownload +libalpm_la_LIBADD = -larchive -ldownload if HAS_DOXYGEN all: doxygen.in diff -Naurp pacman-lib.orig/lib/libalpm/alpm.c pacman-lib.codecleanup/lib/libalpm/alpm.c --- pacman-lib.orig/lib/libalpm/alpm.c 2007-01-11 16:17:06.000000000 -0500 +++ pacman-lib.codecleanup/lib/libalpm/alpm.c 2007-01-17 10:58:00.000000000 -0500 @@ -137,7 +137,7 @@ int alpm_release() /** Register a package database * @param treename the name of the repository - * @return 0 on success, -1 on error (pm_errno is set accordingly) + * @return a pmdb_t* on success (the value), NULL on error */ pmdb_t *alpm_db_register(char *treename) { diff -Naurp pacman-lib.orig/lib/libalpm/be_files.c pacman-lib.codecleanup/lib/libalpm/be_files.c --- pacman-lib.orig/lib/libalpm/be_files.c 2007-01-02 17:29:48.000000000 -0500 +++ pacman-lib.codecleanup/lib/libalpm/be_files.c 2007-01-17 11:10:08.000000000 -0500 @@ -180,7 +180,7 @@ int _alpm_db_read(pmdb_t *db, unsigned i FILE *fp = NULL; struct stat buf; char path[PATH_MAX+1]; - char line[513] = {0}; + char line[513]; pmlist_t *tmplist; char *locale; @@ -205,7 +205,7 @@ int _alpm_db_read(pmdb_t *db, unsigned i _alpm_log(PM_LOG_FUNCTION, _("loading package data for %s : level=%d"), info->name, inforeq); /* clear out 'line', to be certain - and to make valgrind happy */ - memset(line, 513, 0); + memset(line, 0, 513); snprintf(path, PATH_MAX, "%s/%s-%s", db->path, info->name, info->version); if(stat(path, &buf)) { @@ -528,20 +528,20 @@ int _alpm_db_write(pmdb_t *db, pmpkg_t * } if(info->size) { fprintf(fp, "%%SIZE%%\n" - "%ld\n\n", info->size); + "%lu\n\n", info->size); } if(info->reason) { fprintf(fp, "%%REASON%%\n" - "%d\n\n", info->reason); + "%u\n\n", info->reason); } } else { if(info->size) { fprintf(fp, "%%CSIZE%%\n" - "%ld\n\n", info->size); + "%lu\n\n", info->size); } if(info->isize) { fprintf(fp, "%%ISIZE%%\n" - "%ld\n\n", info->isize); + "%lu\n\n", info->isize); } if(info->sha1sum) { fprintf(fp, "%%SHA1SUM%%\n" diff -Naurp pacman-lib.orig/lib/libalpm/conflict.c pacman-lib.codecleanup/lib/libalpm/conflict.c --- pacman-lib.orig/lib/libalpm/conflict.c 2006-11-22 04:03:42.000000000 -0500 +++ pacman-lib.codecleanup/lib/libalpm/conflict.c 2007-01-17 11:12:17.000000000 -0500 @@ -206,6 +206,10 @@ pmlist_t *_alpm_checkconflicts(pmdb_t *d return(baddeps); } +/* Returns a pmlist_t* of file conflicts. + * + * adds list of files to skip to pmlist_t** skip_list. + */ pmlist_t *_alpm_db_find_conflicts(pmdb_t *db, pmtrans_t *trans, char *root, pmlist_t **skip_list) { pmlist_t *i, *j, *k; @@ -230,9 +234,10 @@ pmlist_t *_alpm_db_find_conflicts(pmdb_t for(k = p1->files; k; k = k->next) { filestr = k->data; if(filestr[strlen(filestr)-1] == '/') { - /* this filename has a trailing '/', so it's a directory -- skip it. */ + /* has a trailing '/', so it's a directory -- skip it. */ continue; } + /* TODO do we need both install file names here? */ if(!strcmp(filestr, "._install") || !strcmp(filestr, ".INSTALL")) { continue; } diff -Naurp pacman-lib.orig/lib/libalpm/deps.c pacman-lib.codecleanup/lib/libalpm/deps.c --- pacman-lib.orig/lib/libalpm/deps.c 2006-12-05 02:00:22.000000000 -0500 +++ pacman-lib.codecleanup/lib/libalpm/deps.c 2007-01-17 11:14:27.000000000 -0500 @@ -119,6 +119,10 @@ pmlist_t *_alpm_sortbydeps(pmlist_t *tar while(change) { pmlist_t *tmptargs = NULL; change = 0; + /* TODO only use of a math.h function in entire libalpm, + * can we get rid of it? Former code line: + *if(numscans > numtargs) { + */ if(numscans > sqrt(numtargs)) { _alpm_log(PM_LOG_DEBUG, _("possible dependency cycle detected")); continue; diff -Naurp pacman-lib.orig/lib/libalpm/error.c pacman-lib.codecleanup/lib/libalpm/error.c --- pacman-lib.orig/lib/libalpm/error.c 2006-11-16 12:24:44.000000000 -0500 +++ pacman-lib.codecleanup/lib/libalpm/error.c 2007-01-17 11:14:58.000000000 -0500 @@ -134,7 +134,7 @@ char *alpm_strerror(int err) case PM_ERR_CONF_DIRECTIVE_OUTSIDE_SECTION: return _("all directives must belong to a section"); case PM_ERR_INVALID_REGEX: - return _("valid regular expression"); + return _("invalid regular expression"); case PM_ERR_CONNECT_FAILED: return _("connection to remote host failed"); case PM_ERR_FORK_FAILED: diff -Naurp pacman-lib.orig/lib/libalpm/handle.c pacman-lib.codecleanup/lib/libalpm/handle.c --- pacman-lib.orig/lib/libalpm/handle.c 2006-12-05 02:00:22.000000000 -0500 +++ pacman-lib.codecleanup/lib/libalpm/handle.c 2007-01-17 11:16:02.000000000 -0500 @@ -27,7 +27,6 @@ #include <unistd.h> #include <limits.h> #include <sys/types.h> -#include <stdarg.h> #include <syslog.h> #include <libintl.h> #include <time.h> diff -Naurp pacman-lib.orig/lib/libalpm/list.c pacman-lib.codecleanup/lib/libalpm/list.c --- pacman-lib.orig/lib/libalpm/list.c 2007-01-11 16:17:06.000000000 -0500 +++ pacman-lib.codecleanup/lib/libalpm/list.c 2007-01-17 11:27:46.000000000 -0500 @@ -307,6 +307,7 @@ pmlist_t *_alpm_list_last(pmlist_t *list return(NULL); } + /* TODO this is the only assert.h used in entire library */ assert(list->last != NULL); return(list->last); diff -Naurp pacman-lib.orig/lib/libalpm/md5driver.c pacman-lib.codecleanup/lib/libalpm/md5driver.c --- pacman-lib.orig/lib/libalpm/md5driver.c 2006-10-15 15:31:05.000000000 -0400 +++ pacman-lib.codecleanup/lib/libalpm/md5driver.c 2007-01-17 11:29:33.000000000 -0500 @@ -21,7 +21,6 @@ documentation and/or software. #include <stdlib.h> #include <stdio.h> -#include <time.h> #include <string.h> #include <libintl.h> #include "util.h" diff -Naurp pacman-lib.orig/lib/libalpm/package.c pacman-lib.codecleanup/lib/libalpm/package.c --- pacman-lib.orig/lib/libalpm/package.c 2006-11-22 04:03:42.000000000 -0500 +++ pacman-lib.codecleanup/lib/libalpm/package.c 2007-01-17 11:32:09.000000000 -0500 @@ -26,7 +26,6 @@ #include <stdio.h> #include <stdlib.h> #include <limits.h> -#include <fcntl.h> #include <string.h> #include <libintl.h> #include <locale.h> diff -Naurp pacman-lib.orig/lib/libalpm/server.c pacman-lib.codecleanup/lib/libalpm/server.c --- pacman-lib.orig/lib/libalpm/server.c 2006-11-21 23:25:31.000000000 -0500 +++ pacman-lib.codecleanup/lib/libalpm/server.c 2007-01-17 11:34:28.000000000 -0500 @@ -27,7 +27,7 @@ #include <sys/stat.h> #include <unistd.h> #include <time.h> -#include <sys/time.h> +#include <download.h> /* pacman */ #include "server.h" diff -Naurp pacman-lib.orig/lib/libalpm/sha1.c pacman-lib.codecleanup/lib/libalpm/sha1.c --- pacman-lib.orig/lib/libalpm/sha1.c 2006-12-20 20:53:41.000000000 -0500 +++ pacman-lib.codecleanup/lib/libalpm/sha1.c 2007-01-17 11:34:50.000000000 -0500 @@ -33,7 +33,6 @@ #include <sys/types.h> #include <stdlib.h> -#include <time.h> #include <string.h> /* diff -Naurp pacman-lib.orig/lib/libalpm/sha1.h pacman-lib.codecleanup/lib/libalpm/sha1.h --- pacman-lib.orig/lib/libalpm/sha1.h 2006-12-20 20:53:41.000000000 -0500 +++ pacman-lib.codecleanup/lib/libalpm/sha1.h 2007-01-17 11:37:34.000000000 -0500 @@ -20,23 +20,30 @@ #include <limits.h> #define rol(x,n) ( ((x) << (n)) | ((x) >> (32 -(n))) ) +/* TODO check this comment */ /* The code below is from md5.h (from coreutils), little modifications */ #define UINT_MAX_32_BITS 4294967295U +/* This new ifdef allows splint to not fail on its static code check */ +#ifdef S_SPLINT_S +typedef unsigned int sha_uint32; +#endif +#ifndef S_SPLINT_S #if UINT_MAX == UINT_MAX_32_BITS - typedef unsigned int sha_uint32; +typedef unsigned int sha_uint32; #else #if USHRT_MAX == UINT_MAX_32_BITS - typedef unsigned short sha_uint32; +typedef unsigned short sha_uint32; #else #if ULONG_MAX == UINT_MAX_32_BITS - typedef unsigned long sha_uint32; +typedef unsigned long sha_uint32; #else /* The following line is intended to evoke an error. Using #error is not portable enough. */ #error "Cannot determine unsigned 32-bit data type" #endif #endif #endif +#endif /* We have to make a guess about the integer type equivalent in size to pointers which should always be correct. */ typedef unsigned long int sha_uintptr; diff -Naurp pacman-lib.orig/lib/libalpm/sync.c pacman-lib.codecleanup/lib/libalpm/sync.c --- pacman-lib.orig/lib/libalpm/sync.c 2007-01-03 03:05:13.000000000 -0500 +++ pacman-lib.codecleanup/lib/libalpm/sync.c 2007-01-17 11:40:43.000000000 -0500 @@ -55,6 +55,14 @@ #include "handle.h" #include "server.h" +/* static function declarations */ +static pmsyncpkg_t *find_pkginsync(char *needle, pmlist_t *haystack); +static int istoonew(pmpkg_t *pkg); +static int find_replacements(pmtrans_t *trans, pmdb_t *db_local, + pmlist_t *dbs_sync); +static int pkg_cmp(const void *p1, const void *p2); + + pmsyncpkg_t *_alpm_sync_new(int type, pmpkg_t *spkg, void *data) { pmsyncpkg_t *sync; @@ -121,7 +129,8 @@ static int istoonew(pmpkg_t *pkg) /* Find recommended replacements for packages during a sync. * (refactored from _alpm_sync_prepare) */ -static int find_replacements(pmtrans_t *trans, pmdb_t *db_local, pmlist_t *dbs_sync) +static int find_replacements(pmtrans_t *trans, pmdb_t *db_local, + pmlist_t *dbs_sync) { pmlist_t *i, *j, *k; diff -Naurp pacman-lib.orig/lib/libalpm/util.c pacman-lib.codecleanup/lib/libalpm/util.c --- pacman-lib.orig/lib/libalpm/util.c 2007-01-17 02:43:09.000000000 -0500 +++ pacman-lib.codecleanup/lib/libalpm/util.c 2007-01-17 11:26:35.000000000 -0500 @@ -36,7 +36,6 @@ #ifdef __sun__ #include <alloca.h> #endif -#include <stdarg.h> #include <string.h> #include <unistd.h> #include <errno.h> diff -Naurp pacman-lib.orig/src/pacman/Makefile.am pacman-lib.codecleanup/src/pacman/Makefile.am --- pacman-lib.orig/src/pacman/Makefile.am 2006-12-28 11:54:35.000000000 -0500 +++ pacman-lib.codecleanup/src/pacman/Makefile.am 2007-01-17 10:56:42.000000000 -0500 @@ -17,9 +17,9 @@ pacman_SOURCES = util.c log.c list.c pac pacman_static_SOURCES = $(pacman_SOURCES) pacman_LDADD = -L$(top_srcdir)/lib/libalpm/.libs \ - -ldownload -larchive -lm -lalpm -lssl -lcrypto + -ldownload -lm -lalpm pacman_static_LDADD = -L$(top_srcdir)/lib/libalpm/.libs/ \ - -ldownload -larchive -lm -lalpm -lssl -lcrypto + -ldownload -lm -lalpm pacman_static_LDFLAGS = $(LDFLAGS) -all-static diff -Naurp pacman-lib.orig/src/pacman/downloadprog.c pacman-lib.codecleanup/src/pacman/downloadprog.c --- pacman-lib.orig/src/pacman/downloadprog.c 2006-12-22 02:11:20.000000000 -0500 +++ pacman-lib.codecleanup/src/pacman/downloadprog.c 2007-01-17 11:44:27.000000000 -0500 @@ -39,10 +39,10 @@ #include "conf.h" /* progress bar */ -float rate_last; -int xfered_last; -struct timeval last_time; -struct timeval initial_time; +static float rate_last; +static int xfered_last; +static struct timeval last_time; +static struct timeval initial_time; /* pacman options */ extern config_t *config; @@ -52,13 +52,13 @@ extern config_t *config; void log_progress(const char *filename, int xfered, int total) { - static int lasthash = 0, mouth = 0; - int i, hash; - long chomp = 0; + static unsigned int lasthash = 0, mouth = 0; + unsigned int i, hash; + unsigned int chomp = 0; char *fname, *p; unsigned int maxcols = getcols(); unsigned int progresslen = maxcols - 57; - int percent = ((float)xfered) / ((float)total) * 100; + int percent = (int)((float)xfered) / ((float)total) * 100; struct timeval current_time; float rate = 0.0; unsigned int eta_h = 0, eta_m = 0, eta_s = 0; @@ -87,17 +87,17 @@ void log_progress(const char *filename, if(xfered == total) { /* compute final values */ - rate = total / (total_timediff * 1024); - eta_s = (int)total_timediff; + rate = (float)total / (total_timediff * 1024); + eta_s = (unsigned int)total_timediff; set_output_padding(0); /* shut off padding */ } else if(timediff < UPDATE_SPEED_SEC) { /* we avoid computing the ETA on too small periods of time, so that results are more significant */ return; } else { - rate = (xfered - xfered_last) / (timediff * 1024); - rate = (rate + 2*rate_last) / 3; - eta_s = (total - xfered) / (rate * 1024); + rate = (float)(xfered - xfered_last) / (timediff * 1024); + rate = (float)(rate + 2*rate_last) / 3; + eta_s = (unsigned int)(total - xfered) / (rate * 1024); } rate_last = rate; @@ -130,11 +130,11 @@ void log_progress(const char *filename, rate = 9999.9; } - printf(" %-*s %6dK %#6.1fK/s %02d:%02d:%02d [", FILENAME_TRIM_LEN, fname, xfered/1024, rate, eta_h, eta_m, eta_s); + printf(" %-*s %6dK %#6.1fK/s %02u:%02u:%02u [", FILENAME_TRIM_LEN, fname, xfered/1024, rate, eta_h, eta_m, eta_s); free(fname); - hash = percent*progresslen/100; + hash = (unsigned int)percent*progresslen/100; for(i = progresslen; i > 0; --i) { if(chomp) { if(i > progresslen - hash) { diff -Naurp pacman-lib.orig/src/pacman/list.c pacman-lib.codecleanup/src/pacman/list.c --- pacman-lib.orig/src/pacman/list.c 2006-11-21 23:53:10.000000000 -0500 +++ pacman-lib.codecleanup/src/pacman/list.c 2007-01-17 11:47:30.000000000 -0500 @@ -28,9 +28,11 @@ #include "util.h" #include "list.h" +/* static function declarations */ static list_t *list_last(list_t *list); +static list_t *list_new(void); -list_t *list_new() +static list_t *list_new() { list_t *list = NULL; @@ -124,7 +126,7 @@ void list_display(const char *title, lis if(list) { for(lp = list, cols = len; lp; lp = lp->next) { - int s = strlen((char *)lp->data)+1; + unsigned int s = strlen((char *)lp->data)+1; unsigned int maxcols = getcols(); if(s+cols >= maxcols) { int i; @@ -143,6 +145,7 @@ void list_display(const char *title, lis } } +/* TODO list_display and pmlist_display are almost duplicate code */ void pmlist_display(const char *title, pmlist_t *list) { pmlist_t *lp; @@ -153,7 +156,7 @@ void pmlist_display(const char *title, p if(list) { for(lp = list, cols = len; lp; lp = alpm_list_next(lp)) { - int s = strlen(alpm_list_getdata(lp))+1; + unsigned int s = strlen(alpm_list_getdata(lp))+1; unsigned int maxcols = getcols(); if(s+cols >= maxcols) { int i; diff -Naurp pacman-lib.orig/src/pacman/list.h pacman-lib.codecleanup/src/pacman/list.h --- pacman-lib.orig/src/pacman/list.h 2006-11-20 04:10:25.000000000 -0500 +++ pacman-lib.codecleanup/src/pacman/list.h 2007-01-17 11:47:44.000000000 -0500 @@ -38,7 +38,6 @@ typedef struct __list_t { FREELIST(p); \ } while(0) -list_t *list_new(void); void list_free(list_t *list); list_t *list_add(list_t *list, void *data); int list_count(list_t *list); diff -Naurp pacman-lib.orig/src/pacman/log.c pacman-lib.codecleanup/src/pacman/log.c --- pacman-lib.orig/src/pacman/log.c 2006-12-22 02:11:20.000000000 -0500 +++ pacman-lib.codecleanup/src/pacman/log.c 2007-01-17 11:48:59.000000000 -0500 @@ -39,7 +39,7 @@ extern config_t *config; int neednl = 0; /* for cleaner message output */ -int needpad = 0; /* pad blanks to terminal width */ +static int needpad = 0; /* pad blanks to terminal width */ /* simple helper for needpad */ void set_output_padding(int on) @@ -131,8 +131,9 @@ void pm_fprintf(FILE *file, unsigned sho fprintf(file, str); if(needpad == 1) { + unsigned int i; unsigned int cols = getcols(); - for(int i=len; i < cols; ++i) { + for(i=len; i < cols; ++i) { fprintf(file, " "); } if(neednl == 1) { diff -Naurp pacman-lib.orig/src/pacman/package.c pacman-lib.codecleanup/src/pacman/package.c --- pacman-lib.orig/src/pacman/package.c 2007-01-17 01:07:16.000000000 -0500 +++ pacman-lib.codecleanup/src/pacman/package.c 2007-01-17 11:49:55.000000000 -0500 @@ -218,7 +218,7 @@ void dump_pkg_changelog(char *clfile, co { while(!feof(fp)) { - fgets(line, PATH_MAX, fp); + fgets(line, (int)PATH_MAX, fp); printf("%s", line); line[0] = '\0'; } diff -Naurp pacman-lib.orig/src/pacman/pacman.c pacman-lib.codecleanup/src/pacman/pacman.c --- pacman-lib.orig/src/pacman/pacman.c 2007-01-17 01:07:16.000000000 -0500 +++ pacman-lib.codecleanup/src/pacman/pacman.c 2007-01-17 11:53:56.000000000 -0500 @@ -72,15 +72,15 @@ enum { PM_OP_DEPTEST }; -config_t *config = NULL; +config_t *config; pmdb_t *db_local; /* list of (sync_t *) structs for sync locations */ -list_t *pmc_syncs = NULL; +list_t *pmc_syncs; /* list of targets specified on command line */ -list_t *pm_targets = NULL; +static list_t *pm_targets; -unsigned int maxcols = 80; +static unsigned int maxcols = 80; extern int neednl; @@ -134,8 +134,7 @@ static void usage(int op, char *myname) printf(_(" -l, --list list the contents of the queried package\n")); printf(_(" -m, --foreign list all packages that were not found in the sync db(s)\n")); printf(_(" -o, --owns <file> query the package that owns <file>\n")); - printf(_(" -p, --file pacman will query the package file [package] instead of\n")); - printf(_(" looking in the database\n")); + printf(_(" -p, --file query the package file [package] instead of the database\n")); printf(_(" -s, --search search locally-installed packages for matching strings\n")); } else if(op == PM_OP_SYNC) { printf(_("usage: %s {-S --sync} [options] [package]\n"), myname); @@ -278,7 +277,7 @@ static int parseargs(int argc, char *arg {0, 0, 0, 0} }; char root[PATH_MAX]; - struct stat st = {0}; + struct stat st; while((opt = getopt_long(argc, argv, "ARUFQSTDYr:b:vkhscVfmnoldepiuwyg", opts, &option_index))) { if(opt < 0) { @@ -325,7 +324,7 @@ static int parseargs(int argc, char *arg case 'b': if(stat(optarg, &st) == -1 || !S_ISDIR(st.st_mode)) { pm_fprintf(stderr, NL, _("error: '%s' is not a valid db path\n"), optarg); - exit(1); + exit(EXIT_FAILURE); } alpm_option_set_dbpath(optarg); config->dbpath = alpm_option_get_dbpath(optarg); @@ -481,7 +480,7 @@ int main(int argc, char *argv[]) } else { ERR(NL, _("you cannot perform this operation unless you are root.\n")); config_free(config); - exit(1); + exit(EXIT_FAILURE); } } } diff -Naurp pacman-lib.orig/src/pacman/sync.c pacman-lib.codecleanup/src/pacman/sync.c --- pacman-lib.orig/src/pacman/sync.c 2007-01-17 01:07:16.000000000 -0500 +++ pacman-lib.codecleanup/src/pacman/sync.c 2007-01-17 10:46:21.000000000 -0500 @@ -212,6 +212,8 @@ static int sync_synctree(int level, list if(ret < 0) { if(pm_errno == PM_ERR_DB_SYNC) { /* use libdownload error */ + /* TODO breaking abstraction barrier here? + * pacman -> libalpm -> libdownload */ ERR(NL, _("failed to synchronize %s: %s\n"), sync->treename, downloadLastErrString); } else { ERR(NL, _("failed to update %s (%s)\n"), sync->treename, alpm_strerror(pm_errno)); diff -Naurp pacman-lib.orig/src/pacman/trans.c pacman-lib.codecleanup/src/pacman/trans.c --- pacman-lib.orig/src/pacman/trans.c 2006-12-29 12:07:34.000000000 -0500 +++ pacman-lib.codecleanup/src/pacman/trans.c 2007-01-17 11:54:16.000000000 -0500 @@ -41,7 +41,7 @@ extern config_t *config; -int prevpercent=0; /* for less progressbar output */ +static int prevpercent=0; /* for less progressbar output */ /* Callback to handle transaction events */ diff -Naurp pacman-lib.orig/src/pacman/util.c pacman-lib.codecleanup/src/pacman/util.c --- pacman-lib.orig/src/pacman/util.c 2006-12-28 12:20:41.000000000 -0500 +++ pacman-lib.codecleanup/src/pacman/util.c 2007-01-17 11:58:14.000000000 -0500 @@ -30,7 +30,6 @@ #include "config.h" #include <stdio.h> #include <stdlib.h> -#include <stdarg.h> #include <string.h> #include <errno.h> #include <fcntl.h> @@ -52,7 +51,7 @@ extern config_t *config; extern int neednl; /* gets the current screen column width */ -int getcols() +unsigned int getcols() { if(!isatty(1)) { /* We will default to 80 columns if we're not a tty @@ -60,19 +59,21 @@ int getcols() */ return 80; } + else { /* adding else so splint can parse */ #ifdef TIOCGSIZE - struct ttysize win; - if(ioctl(1, TIOCGSIZE, &win) == 0) { - return win.ts_cols; - } + struct ttysize win; + if(ioctl(1, TIOCGSIZE, &win) == 0) { + return win.ts_cols; + } #elif defined(TIOCGWINSZ) - struct winsize win; - if(ioctl(1, TIOCGWINSZ, &win) == 0) { - return win.ws_col; - } + struct winsize win; + if(ioctl(1, TIOCGWINSZ, &win) == 0) { + return win.ws_col; + } #endif - else { - return -1; + else { + return -1; + } } /* Original envvar way - prone to display issues const char *cenv = getenv("COLUMNS"); @@ -155,15 +156,14 @@ int rmrf(char *path) } return(errflag); } - return(0); } /* output a string, but wrap words properly with a specified indentation */ -void indentprint(const char *str, int indent) +void indentprint(const char *str, unsigned int indent) { const char *p = str; - int cidx = indent; + unsigned int cidx = indent; while(*p) { if(*p == ' ') { @@ -178,7 +178,7 @@ void indentprint(const char *str, int in len = next - p; if(len > (getcols()-cidx-1)) { /* newline */ - int i; + unsigned int i; fprintf(stdout, "\n"); for(i = 0; i < indent; i++) { fprintf(stdout, " "); @@ -200,7 +200,7 @@ void indentprint(const char *str, int in char *buildstring(list_t *strlist) { char *str; - int size = 1; + size_t size = 1; list_t *lp; for(lp = strlist; lp; lp = lp->next) { @@ -208,7 +208,7 @@ char *buildstring(list_t *strlist) } str = (char *)malloc(size); if(str == NULL) { - ERR(NL, _("failed to allocated %d bytes\n"), size); + ERR(NL, _("failed to allocate %d bytes\n"), size); } str[0] = '\0'; for(lp = strlist; lp; lp = lp->next) { diff -Naurp pacman-lib.orig/src/pacman/util.h pacman-lib.codecleanup/src/pacman/util.h --- pacman-lib.orig/src/pacman/util.h 2006-11-20 04:10:25.000000000 -0500 +++ pacman-lib.codecleanup/src/pacman/util.h 2007-01-17 11:58:57.000000000 -0500 @@ -28,7 +28,7 @@ p = malloc(b); \ if (!(p)) { \ fprintf(stderr, "malloc failure: could not allocate %d bytes\n", (int)b); \ - exit(1); \ + exit(EXIT_FAILURE); \ } \ } else { \ p = NULL; \ @@ -43,10 +43,10 @@ } while(0) #define _(str) gettext(str) -int getcols(); +unsigned int getcols(); int makepath(char *path); int rmrf(char *path); -void indentprint(const char *str, int indent); +void indentprint(const char *str, unsigned int indent); char *buildstring(list_t *strlist); char *strtoupper(char *str); char *strtrim(char *str);
On 1/17/07, Dan McGee <dpmcgee@gmail.com> wrote:
/* TODO only use of a math.h function in entire libalpm, * can we get rid of it? Former code line: *if(numscans > numtargs) { */ if(numscans > sqrt(numtargs)) {
Interesting point. It does appear that the sqrt is merely an optimization. We could probably use numtargs/2 and achieve much the same overall speedup, as 'N' in these loops is probably in the 100-1000 range, this won't be a bottleneck. Might be worth a test though. Still, I have no real opinion on whether the removal of libm as a dep is worth it or not.
On Wed, Jan 17, 2007 at 03:27:33PM -0600, Aaron Griffin <aaronmgriffin@gmail.com> wrote:
On 1/17/07, Dan McGee <dpmcgee@gmail.com> wrote:
/* TODO only use of a math.h function in entire libalpm, * can we get rid of it? Former code line: *if(numscans > numtargs) { */ if(numscans > sqrt(numtargs)) {
Interesting point. It does appear that the sqrt is merely an optimization.
yes, it is. sorting ~3000 pkgs on my machine now takes ~45sec while before it "never ended" (probably it would after a few hours) if you want to add a test, then just add all the packages in the sync repo to a transaction using the noconfilcts option then you'll see how much time will the sort take with numtargs/2 udv / greetings, VMiklos -- Developer of Frugalware Linux, to make things frugal - http://frugalware.org
On 1/17/07, Dan McGee <dpmcgee@gmail.com> wrote:
* Removed some unnecessary headers and library links * Made things static if possible * Cleaned up makefiles a bit * Fixed some old comments in the code * Fixed some errors the static code checker splint pointed out (unnecessary = NULL, bad exit() syntax, casting problems, etc.) * Backwards arguments in a memset call in _alpm_db_read (could have been worse) * Other various small fixes
Signed-off-by: Dan McGee <dpmcgee@gmail.com>
- memset(line, 513, 0); + memset(line, 0, 513); This made me laugh. Feel free to stab me for that one. if(!strcmp(filestr, "._install") || !strcmp(filestr, ".INSTALL")) { As per your comment, I removed the ._install holdover from some older
Merged, with the following changes / comments: package version (if there are any around still, I'd be amazed)
-return _("valid regular expression"); +return _("invalid regular expression"); Funniest typo ever.
assert(list->last != NULL);
I just removed this. Sure, asserts are nice, but this returns list->last anyway. Calling functions can test for validity.
+/* static function declarations */ +static pmsyncpkg_t *find_pkginsync(char *needle, pmlist_t *haystack); +static int istoonew(pmpkg_t *pkg); +static int find_replacements(pmtrans_t *trans, pmdb_t *db_local, + pmlist_t *dbs_sync); +static int pkg_cmp(const void *p1, const void *p2);
I left this out for now. While it may turn out messy, I'm not a huge fan of declarations like this for all internal functions, only when it actually requires a forward-decl. Still, it's purely cosmetic, so I can add it back in if you want.
- -ldownload -larchive -lm -lalpm -lssl -lcrypto + -ldownload -lm -lalpm
Odd. This may be a build error on my part, but libdownload should require libssl/libcrypto for https support.
diff -Naurp pacman-lib.orig/src/pacman/list.c pacman-lib.codecleanup/src/pacman/list.c
I left this file untouched. Reason being that tonight I'm going to drop it entirely for alpm_list_t (as per another thread).
+ unsigned int i; unsigned int cols = getcols(); - for(int i=len; i < cols; ++i) { + for(i=len; i < cols; ++i) {
Just FTR: pacman3 should be C99 compliant, so declaring the index variable outside the for loop shouldn't be required, though again, it's purely cosmetics at this point 8).
+static unsigned int maxcols = 80; Unused. Removed this. getcols is used instead.
+/* TODO breaking abstraction barrier here? + * pacman -> libalpm -> libdownload */ See code for my additions to this comment.
Other than that it's all cool. Committing now.
On 1/18/07, Aaron Griffin <aaronmgriffin@gmail.com> wrote:
On 1/17/07, Dan McGee <dpmcgee@gmail.com> wrote: Merged, with the following changes / comments:
- memset(line, 513, 0); + memset(line, 0, 513); This made me laugh. Feel free to stab me for that one. I knew you added it but refrained from harassing comments. :)
-return _("valid regular expression"); +return _("invalid regular expression"); Funniest typo ever. That was actually a downstream Frugalware patch, can't say I noticed it until I saw it there.
+/* static function declarations */ I left this out for now. While it may turn out messy, I'm not a huge fan of declarations like this for all internal functions, only when it actually requires a forward-decl. Still, it's purely cosmetic, so I can add it back in if you want. I just did it for consistency; that way you never have to worry about it being forward/backward. The other proposal would be to say static functions should _always_ go first in a file. That would remove the need for declarations. I just feel like consistency is important.
- -ldownload -larchive -lm -lalpm -lssl -lcrypto + -ldownload -lm -lalpm Odd. This may be a build error on my part, but libdownload should require libssl/libcrypto for https support. Self-explanatory: $ ldd /usr/lib/libdownload.so linux-gate.so.1 => (0xffffe000) libc.so.6 => /lib/libc.so.6 (0xb7de9000) /lib/ld-linux.so.2 (0x80000000)
+ unsigned int i; unsigned int cols = getcols(); - for(int i=len; i < cols; ++i) { + for(i=len; i < cols; ++i) {
Just FTR: pacman3 should be C99 compliant, so declaring the index variable outside the for loop shouldn't be required, though again, it's purely cosmetics at this point 8). Splint didn't like it, it died at it so it was easy enough to fix. I figured it could be fixed in the codebase too since I'm sure gcc ends up doing exactly the same thing with it anyway.
-Dan
On the topic of code cleanup, I've only seen it once but I noticed a piece of libtar code commented it out, and I see no reason behind its purpose being there (its in libalpm's add.c). For all I know, this might be the only case of libtar comments, but if there are others of these, I can only see their removal helping readability of alpm code. ~ Jamie / yankees26
participants (4)
-
Aaron Griffin
-
Dan McGee
-
James Rosten
-
VMiklos