svn commit: r230037 - head/lib/libutil

[Guy Helmer]

On Jan 17, 2012, at 3:58 AM, Bruce Evans wrote:

> On Mon, 16 Jan 2012, Pawel Jakub Dawidek wrote:
> 
>> On Mon, Jan 16, 2012 at 11:14:32AM -0600, Guy Helmer wrote:
>>> I've pasted the diff below that I think captures the majority of the issues you have brought up. I have not attempted to tackle the property.3/properties.3 issues, nor the objections to the prefixes that I think would take considerably more effort to resolve -- I wanted to concentrate on the issues that can be isolated to libutil. I hope the diff was pasted OK, especially WRT <tab> characters.
>> 
>> The patch looks mostly good, one nit mentioned below and also one
>> question for Bruce.
>> 
>>> +/* Flags for hexdump(3) */
>>> +/* Flags for humanize_number(3) flags */
>>> +/* Flags for humanize_number(3) scale */
>>> +/* return values from realhostname() */
>>> +/* Flags for pw_scan() */
>>> +/* Return values from uu_lock() */
>> 
>> All those sentences are missing period and one doesn't start with
>> capital letter.
> 
> I decided not to worry about the termination since it was fairly consistent
> in the file.  The most recent commit fixed these, but made all the others
> inconsistent:
> 
> % /* for properties.c */
> % /* Avoid pulling in all the include files for no need */
> % #ifdef _STDIO_H_	/* avoid adding new includes */
> 
> This is now the only comment to the right of code.  Comments to the
> right of ifdefs are especially hard format nicely
>    (the normal comment indentation to 32 or 40 columns doesn't work
>    well; I normally use a single space; the above indents to 24 columns),
> so the should be avoided.  The same treatment is used for 3 other includes,
> but there is no comment for the others.  The simplest fix is to remove
> this comment.  Otherwise, put a meta-comment about them all somewhere.
> It's hard to place this so that it clearly covers them all, but anywhere
> is better than to the right of 1 of their ifdefs.
> 
> % /* fparseln(3) */
> 
> This is also missing the new, otherwise (too) uniform wording
> "Flags for..."
> 
>> I noticed also one more inconsistency:
>> 
>> struct kinfo_file *
>> 	kinfo_getfile(pid_t _pid, int *_cntp);
>> struct passwd
>> 	*pw_dup(const struct passwd *_pw);
>> 
>> Sometimes * is on the same line as function type and sometimes it is in
>> the line below. Former is definiately better.
> 
> Indded.
> 

OK, one more round of proposed changes:

Index: lib/libutil/libutil.h
===================================================================
--- lib/libutil/libutil.h	(revision 230318)
+++ lib/libutil/libutil.h	(working copy)
@@ -71,14 +71,14 @@
 #define	PROPERTY_MAX_NAME	64
 #define	PROPERTY_MAX_VALUE	512
 
-/* for properties.c */
+/* For properties.c. */
 typedef struct _property {
 	struct _property *next;
 	char	*name;
 	char	*value;
 } *properties;
 
-/* Avoid pulling in all the include files for no need */
+/* Avoid pulling in all the include files for no need. */
 struct in_addr;
 struct pidfh;
 struct sockaddr;
@@ -132,7 +132,11 @@
 int	uu_unlock(const char *_ttyname);
 int	uu_lock_txfr(const char *_ttyname, pid_t _pid);
 
-#ifdef _STDIO_H_	/* avoid adding new includes */
+/*
+ * Conditionally prototype the following functions if the include
+ * files upon which they depend have been included.
+ */
+#ifdef _STDIO_H_
 char	*fparseln(FILE *_fp, size_t *_len, size_t *_lineno,
 	    const char _delim[3], int _flags);
 #endif
@@ -150,26 +154,26 @@
 char	*pw_make_v7(const struct passwd *_pw);
 int	pw_mkdb(const char *_user);
 int	pw_lock(void);
-struct passwd
-	*pw_scan(const char *_line, int _flags);
-const char
-	*pw_tempname(void);
+struct passwd *
+	pw_scan(const char *_line, int _flags);
+const char *
+	pw_tempname(void);
 int	pw_tmp(int _mfd);
 #endif
 
 #ifdef _GRP_H_
 int 	gr_copy(int __ffd, int _tfd, const struct group *_gr,
 	    struct group *_old_gr);
-struct group
-	*gr_dup(const struct group *_gr);
+struct group *
+	gr_dup(const struct group *_gr);
 int	gr_equal(const struct group *_gr1, const struct group *_gr2);
 void	gr_fini(void);
 int	gr_init(const char *_dir, const char *_master);
 int	gr_lock(void);
 char	*gr_make(const struct group *_gr);
 int	gr_mkdb(void);
-struct group
-	*gr_scan(const char *_line);
+struct group *
+	gr_scan(const char *_line);
 int	gr_tmp(int _mdf);
 #endif
 
@@ -209,18 +213,18 @@
 #define	HD_OMIT_HEX		(1 << 17)
 #define	HD_OMIT_CHARS		(1 << 18)
 
-/* Flags for humanize_number(3) flags. */
+/* Values for humanize_number(3)'s flags parameter. */
 #define	HN_DECIMAL		0x01
 #define	HN_NOSPACE		0x02
 #define	HN_B			0x04
 #define	HN_DIVISOR_1000		0x08
 #define	HN_IEC_PREFIXES		0x10
 
-/* Flags for humanize_number(3) scale. */
+/* Values for humanize_number(3)'s scale parameter. */
 #define	HN_GETSCALE		0x10
 #define	HN_AUTOSCALE		0x20
 
-/* return values from realhostname(). */
+/* Return values from realhostname(). */
 #define	HOSTNAME_FOUND		0
 #define	HOSTNAME_INCORRECTNAME	1
 #define	HOSTNAME_INVALIDADDR	2
@@ -233,12 +237,12 @@
 /* Return values from uu_lock(). */
 #define	UU_LOCK_INUSE		1
 #define	UU_LOCK_OK		0
-#define	UU_LOCK_OPEN_ERR	-1
-#define	UU_LOCK_READ_ERR	-2
-#define	UU_LOCK_CREAT_ERR	-3
-#define	UU_LOCK_WRITE_ERR	-4
-#define	UU_LOCK_LINK_ERR	-5
-#define	UU_LOCK_TRY_ERR		-6
-#define	UU_LOCK_OWNER_ERR	-7
+#define	UU_LOCK_OPEN_ERR	(-1)
+#define	UU_LOCK_READ_ERR	(-2)
+#define	UU_LOCK_CREAT_ERR	(-3)
+#define	UU_LOCK_WRITE_ERR	(-4)
+#define	UU_LOCK_LINK_ERR	(-5)
+#define	UU_LOCK_TRY_ERR		(-6)
+#define	UU_LOCK_OWNER_ERR	(-7)
 
 #endif /* !_LIBUTIL_H_ */


--------
This message has been scanned by ComplianceSafe, powered by Palisade's PacketSure.