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.
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
