Hello pacman-hackers, I am quite new to this list and to ArchLinux in general, so if I'm talking crap, please correct me and don't take all this as an offence. The problem I'm concerned about is the general style of writing code in the library. There are some inconsistencies I found and I would like to discuss them with you. * Casting of return values Not needed in general, sometimes done - especially on malloc() calls. Since it's not needed, it would be better to leave it out, because its syntax decreases readability. * More defense return value checking I found unchecked strdup() value using e.g. * Better use of library functions In the attached patch you can see what I mean. Using one memset call to nullify a region instead of ~30 assignments. * Linewidth and indentation This is a hot topic. Some people's argument is, that they don't want to code on 80 char wide terminals when there is a 2048x1024 resolution on a 20" screen. However, I think that /usr/src/linux/Documentation/CodingStyle has a good point on that. It's not just an optical problem, it's design. I'm not trying to piss anyone or just rant without doing something. If you agree that a coding standard would be an overall benefit, I will help creating one and applying it to the code base. Chaotic greetings, Hannes Index: lib/libalpm/package.c =================================================================== RCS file: /home/cvs-pacman/pacman-lib/lib/libalpm/package.c,v retrieving revision 1.42 diff -r1.42 package.c 45c45 < pmpkg_t* pkg = NULL; ---
pmpkg_t* pkg; 47c47,51 < if((pkg = (pmpkg_t *)malloc(sizeof(pmpkg_t))) == NULL) {
/* Use calloc to nullify the region at once */ pkg = calloc(sizeof(*pkg)); if (pkg == NULL) { _alpm_log(PM_LOG_ERROR, _("malloc failure: could not allocate " "%d bytes"), sizeof(*pkg)); 53,54d56 < } else { < pkg->name[0] = '\0'; 55a58
58,59d60 < } else { < pkg->version[0] = '\0'; 61,90d61 < pkg->filename[0] = '\0'; < pkg->desc[0] = '\0'; < pkg->url[0] = '\0'; < pkg->license = NULL; < pkg->desc_localized = NULL; < pkg->builddate[0] = '\0'; < pkg->buildtype[0] = '\0'; < pkg->installdate[0] = '\0'; < pkg->packager[0] = '\0'; < pkg->md5sum[0] = '\0'; < pkg->sha1sum[0] = '\0'; < pkg->arch[0] = '\0'; < pkg->size = 0; < pkg->isize = 0; < pkg->scriptlet = 0; < pkg->force = 0; < pkg->reason = PM_PKG_REASON_EXPLICIT; < pkg->requiredby = NULL; < pkg->conflicts = NULL; < pkg->files = NULL; < pkg->backup = NULL; < pkg->depends = NULL; < pkg->removes = NULL; < pkg->groups = NULL; < pkg->provides = NULL; < pkg->replaces = NULL; < /* internal */ < pkg->origin = 0; < pkg->data = NULL; < pkg->infolevel = 0; 97c68 < pmpkg_t* newpkg = NULL; ---
pmpkg_t* newpkg; 99c70 < newpkg = (pmpkg_t *)malloc(sizeof(pmpkg_t));
newpkg = malloc(sizeof(*newpkg)); 101c72,73 < _alpm_log(PM_LOG_ERROR, _("malloc failure: could not allocate %d bytes"), sizeof(pmpkg_t));
_alpm_log(PM_LOG_ERROR, _("malloc failure: could not allocate " "%d bytes"), sizeof(*pkg));
105,121c77,80 < STRNCPY(newpkg->filename, pkg->filename, PKG_FILENAME_LEN); < STRNCPY(newpkg->name, pkg->name, PKG_NAME_LEN); < STRNCPY(newpkg->version, pkg->version, PKG_VERSION_LEN); < STRNCPY(newpkg->desc, pkg->desc, PKG_DESC_LEN); < STRNCPY(newpkg->url, pkg->url, PKG_URL_LEN); < STRNCPY(newpkg->builddate, pkg->builddate, PKG_DATE_LEN); < STRNCPY(newpkg->buildtype, pkg->buildtype, PKG_DATE_LEN); < STRNCPY(newpkg->installdate, pkg->installdate, PKG_DATE_LEN); < STRNCPY(newpkg->packager, pkg->packager, PKG_PACKAGER_LEN); < STRNCPY(newpkg->md5sum, pkg->md5sum, PKG_MD5SUM_LEN); < STRNCPY(newpkg->sha1sum, pkg->sha1sum, PKG_SHA1SUM_LEN); < STRNCPY(newpkg->arch, pkg->arch, PKG_ARCH_LEN); < newpkg->size = pkg->size; < newpkg->isize = pkg->isize; < newpkg->force = pkg->force; < newpkg->scriptlet = pkg->scriptlet; < newpkg->reason = pkg->reason; ---
/* Copy over the whole regions but take care of the lists manually. */ memcpy(newpkg, pkg, sizeof(*newpkg));
132a92
134c94 < newpkg->origin = pkg->origin; ---
/* XXX: strdup return value? */ 136d95 < newpkg->infolevel = pkg->infolevel; 324,328c283 < pm_errno = PM_ERR_PKG_INVALID; < unlink(descfile); < FREE(descfile); < close(fd); < goto error;
goto pkg_invalid;
332,336c287 < pm_errno = PM_ERR_PKG_INVALID; < unlink(descfile); < FREE(descfile); < close(fd); < goto error; ---
goto pkg_invalid;
340,344c291 < pm_errno = PM_ERR_PKG_INVALID; < unlink(descfile); < FREE(descfile); < close(fd); < goto error; ---
goto pkg_invalid;
413a361,366
pkg_invalid: pm_errno = PM_ERR_PKG_INVALID; unlink(descfile); FREE(descfile); close(fd);