[pacman-dev] Codingstyle

[Johannes Weiner]

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
-------------- next part --------------
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);
>