Date: Tue, 3 Feb 2015 22:29:57 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Vsevolod Stakhov <vsevolod@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r278081 - head/sbin/ifconfig Message-ID: <20150203213023.I1264@besplex.bde.org> In-Reply-To: <201502021437.t12Ebk7M002347@svn.freebsd.org> References: <201502021437.t12Ebk7M002347@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 2 Feb 2015, Vsevolod Stakhov wrote:
> Log:
> Style(9) fixes.
This is still a fair way from style(9) even on the changed lines.
> Modified: head/sbin/ifconfig/af_inet6.c
> ==============================================================================
> --- head/sbin/ifconfig/af_inet6.c Mon Feb 2 13:03:04 2015 (r278080)
> +++ head/sbin/ifconfig/af_inet6.c Mon Feb 2 14:37:45 2015 (r278081)
> @@ -58,8 +58,8 @@ static const char rcsid[] =
> #include "ifconfig.h"
>
> static struct in6_ifreq in6_ridreq;
> -static struct in6_aliasreq in6_addreq =
> - { .ifra_flags = 0,
> +static struct in6_aliasreq in6_addreq =
> + { .ifra_flags = 0,
> .ifra_lifetime = { 0, 0, ND6_INFINITE_LIFETIME, ND6_INFINITE_LIFETIME } };
In strict KNF, the type name is indented 1 tab after the storage class.
That rule is followed here.
In old strict KNF, the second work of long type declartor is indented 1 tab.
That would be after 'struct'. FreeBSD removed this rule. It is not
followed here. The formatting now follows the FreeBSD rule of just
space separators in the type declarator.
In not so strict KNF, the variable name is indented by 1 tab after the
type declarator. FreeBSD broke this rule to allow any number of tabs
so as to line up all the variable names, at least for struct members,
even if this puts the names far to the right leaving not enough space
for comments, provided there are more than just 10% of names that would
not line up with minimal indentation. This rule (with less than
maximal indentation) is more usefule than the others. It is not followed
here.
In KNF (or just 1TBS) the left brace goes on the previous line.
Indentation in KNF is 1 tab, not 4 spaces as it was before the change and
2 spaces afer the change.
In KNF (or just myNF), lists in initializaers should be terminated with a
comma.
In KNF (or just 1TBS), the last right brace goes on a line by itself,
with indentation by a number of tabs (possibly 0).
Indentation of the storage class but not anything else makes the
(implicit) rules for indentation of the initializers unclear. For
file-scope declarations, it is common to put the trailing brace in
column 0 and indent relative to that. However, for prototypes, the
following indentation is normal. From <stdlib.h>
void qsort(void *, size_t, size_t,
int (*)(const void *, const void *));
Note that the function name is indented with strict KNF to a fault --
by 1 tab and 1 space (the space is to allow for a single star before
the function name). Then if the prototype needs multiple lines, they
are indented relative to the function name. The analogue of indenting
relative to the variable name in the above gives after fixing all the
style bugs except for the extra space before the variable name:
static struct in6_aliasreq in6_addreq = {
.ifra_flags = 0,
.ifra_lifetime = { 0, 0,
ND6_INFINITE_LIFETIME,
ND6_INFINITE_LIFETIME,
},
};
but that is too verbose even when the variable name starts less than
half way to the right, so we should indent the initializer relative
to column 0 and cheat by omitting the optional commas:
static struct in6_aliasreq in6_addreq = {
.ifra_flags = 0,
.ifra_lifetime = { 0, 0, ND6_INFINITE_LIFETIME, ND6_INFINITE_LIFETIME }
};
Many other initializers need special formatting like this, that is hard to
automatically generate or check. It is much easier to automaticallY
format complicated statements than simple initializers since code has
more structure.
> static int ip6lifetime;
>
> @@ -284,20 +284,23 @@ in6_status(int s __unused, const struct
> if ((flags6 & IN6_IFF_PREFER_SOURCE) != 0)
> printf("prefer_source ");
>
> - in6_print_scope((uint8_t *)&((struct sockaddr_in6 *)(ifa->ifa_addr))->sin6_addr);
> + in6_print_scope((uint8_t *)&((struct sockaddr_in6 *)
> + (ifa->ifa_addr))->sin6_addr);
The formatting is OK. Can the bogus casts be improved?
>
> if (ip6lifetime && (lifetime.ia6t_preferred || lifetime.ia6t_expire)) {
> printf("pltime ");
> if (lifetime.ia6t_preferred) {
> printf("%s ", lifetime.ia6t_preferred < now.tv_sec
> - ? "0" : sec2str(lifetime.ia6t_preferred - now.tv_sec));
> + ? "0" :
> + sec2str(lifetime.ia6t_preferred - now.tv_sec));
"?" at the start of a line is still gnu style. The KNF formatting is:
printf("%s ",
lifetime.ia6t_preferred < now.tv_sec ? "0" :
sec2str(lifetime.ia6t_preferred - now.tv_sec));
> } else
> printf("infty ");
>
> printf("vltime ");
> if (lifetime.ia6t_expire) {
> printf("%s ", lifetime.ia6t_expire < now.tv_sec
> - ? "0" : sec2str(lifetime.ia6t_expire - now.tv_sec));
> + ? "0" :
> + sec2str(lifetime.ia6t_expire - now.tv_sec));
Similarly.
> } else
> printf("infty ");
> }
> @@ -372,25 +375,25 @@ in6_getaddr(const char *s, int which)
> static int
> prefix(void *val, int size)
> {
> - u_char *name = (u_char *)val;
> - int byte, bit, plen = 0;
> + u_char *name = (u_char *)val;
> + int byte, bit, plen = 0;
The following style bugs were not changed:
- the case to u_char * is bogus (has no effect). This is not C++.
- initialization in declarations (2 instances)
- unsorted declarations (1 instance).
>
> - for (byte = 0; byte < size; byte++, plen += 8)
> - if (name[byte] != 0xff)
> - break;
> + for (byte = 0; byte < size; byte++, plen += 8)
> + if (name[byte] != 0xff)
> + break;
> if (byte == size)
> return (plen);
> for (bit = 7; bit != 0; bit--, plen++)
> - if (!(name[byte] & (1 << bit)))
> - break;
> - for (; bit != 0; bit--)
> - if (name[byte] & (1 << bit))
> - return(0);
> - byte++;
> - for (; byte < size; byte++)
> - if (name[byte])
> - return(0);
> - return (plen);
> + if (!(name[byte] & (1 << bit)))
> + break;
> + for (; bit != 0; bit--)
> + if (name[byte] & (1 << bit))
> + return(0);
> + byte++;
> + for (; byte < size; byte++)
> + if (name[byte])
> + return(0);
> + return (plen);
OK, except:
- most but not all returns are still missing a space before the left
parenthesis
- 2 tests of name[byte] are boolean in non-boolean context. Strict
KNF may even require testing (foo & MASK) as non-boolean. I don't
like that.
> }
>
> static char *
> @@ -534,7 +537,11 @@ in6_Lopt_cb(const char *optarg __unused)
> {
> ip6lifetime++; /* print IPv6 address lifetime */
> }
> -static struct option in6_Lopt = { .opt = "L", .opt_usage = "[-L]", .cb = in6_Lopt_cb };
> +static struct option in6_Lopt = {
> + .opt = "L",
> + .opt_usage = "[-L]",
> + .cb = in6_Lopt_cb
> +};
This is a more normal style for initializers. They are hard enough to
format without tabs here and there.
C99 initializers allow sorting the initializers in a less random order
than the struct members. This doesn't seem to be taken advantage of here.
>
> static __constructor void
> inet6_ctor(void)
>
> Modified: head/sbin/ifconfig/ifconfig.c
> ==============================================================================
> --- head/sbin/ifconfig/ifconfig.c Mon Feb 2 13:03:04 2015 (r278080)
> +++ head/sbin/ifconfig/ifconfig.c Mon Feb 2 14:37:45 2015 (r278081)
> @@ -151,12 +151,14 @@ usage(void)
> exit(1);
> }
>
> +#define ORDERS_SIZE(x) sizeof(x) / sizeof(x[0])
> +
Non-KNF indentation.
Missing parentheses around one x.
Non-use of the standard macro for this. FreeBSD has about 2 versions of
it and is slowly standardizing one. Elsewhere the cleanups were to replace
home made versions by the standard one.
> static int
> calcorders(struct ifaddrs *ifa, struct ifa_queue *q)
> {
> - unsigned int ord, af, ifa_ord;
> struct ifaddrs *prev;
> struct ifa_order_elt *cur;
> + unsigned int ord, af, ifa_ord;
Now the unsorting is only within the u_ints.
'unsigned int' is still not spelled u_int.
>
> prev = NULL;
> cur = NULL;
Example of the KNF rule of no initialization in declarations.
> @@ -164,7 +166,8 @@ calcorders(struct ifaddrs *ifa, struct i
> ifa_ord = 0;
>
> while (ifa != NULL) {
> - if (prev == NULL || strcmp(ifa->ifa_name, prev->ifa_name) != 0) {
> + if (prev == NULL ||
> + strcmp(ifa->ifa_name, prev->ifa_name) != 0) {
> cur = calloc(1, sizeof(*cur));
>
> if (cur == NULL)
OK.
> @@ -179,12 +182,12 @@ calcorders(struct ifaddrs *ifa, struct i
> if (ifa->ifa_addr) {
> af = ifa->ifa_addr->sa_family;
>
> - if (af < sizeof(cur->af_orders) / sizeof(cur->af_orders[0]) &&
> - cur->af_orders[af] == 0)
> + if (af < ORDERS_SIZE(cur->af_orders) &&
> + cur->af_orders[af] == 0)
> cur->af_orders[af] = ++ord;
OK except for non-use of the standard macro. The name of the special macro
is also too specialized. It is not limited to variables named 'orders'.
> }
> prev = ifa;
> - ifa = ifa->ifa_next;
> + ifa = ifa->ifa_next;
> }
>
> return (0);
> @@ -193,15 +196,14 @@ calcorders(struct ifaddrs *ifa, struct i
> static int
> cmpifaddrs(struct ifaddrs *a, struct ifaddrs *b, struct ifa_queue *q)
> {
> - int ret;
> - unsigned int af1, af2;
> struct ifa_order_elt *cur, *e1, *e2;
> + unsigned int af1, af2;
> + int ret;
OK except for missing conversion to u_int.
>
> e1 = e2 = NULL;
>
> ret = strcmp(a->ifa_name, b->ifa_name);
> if (ret != 0) {
> - /* We need to find elements corresponding to these different names */
Possibly not banal; just too long for 1 line, and missing punctuation.
> TAILQ_FOREACH(cur, q, link) {
> if (e1 && e2)
> break;
> @@ -231,20 +233,21 @@ cmpifaddrs(struct ifaddrs *a, struct ifa
> af1 = a->ifa_addr->sa_family;
> af2 = b->ifa_addr->sa_family;
>
> - if (af1 < sizeof(e1->af_orders) / sizeof(e1->af_orders[0]) &&
> - af2 < sizeof(e1->af_orders) / sizeof(e1->af_orders[0]))
> + if (af1 < ORDERS_SIZE(e1->af_orders) &&
> + af2 < ORDERS_SIZE(e1->af_orders))
> return (e1->af_orders[af2] - e1->af_orders[af1]);
OK, except as above.
> }
>
> return (0);
> }
>
> +#undef ORDERS_SIZE
> +
> static struct ifaddrs *
> sortifaddrs(struct ifaddrs *list,
> - int (*compare)(struct ifaddrs *, struct ifaddrs *, struct ifa_queue *),
> - struct ifa_queue *q)
> + int (*compare)(struct ifaddrs *, struct ifaddrs *, struct ifa_queue *),
> + struct ifa_queue *q)
Seems hard to avoid this ugliness.
> {
> -
> struct ifaddrs *right, *temp, *last, *result, *next, *tail;
>
> right = list;
> @@ -499,7 +502,8 @@ main(int argc, char *argv[])
> sdl->sdl_alen != ETHER_ADDR_LEN)
> continue;
> } else {
> - if (ifa->ifa_addr->sa_family != afp->af_af)
> + if (ifa->ifa_addr->sa_family
> + != afp->af_af)
Gnu-style placement of operator.
> continue;
> }
> }
> @@ -835,7 +839,7 @@ settunnel(const char *src, const char *d
> errx(1, "error in parsing address string: %s",
> gai_strerror(ecode));
>
> - if ((ecode = getaddrinfo(dst, NULL, NULL, &dstres)) != 0)
> + if ((ecode = getaddrinfo(dst, NULL, NULL, &dstres)) != 0)
> errx(1, "error in parsing address string: %s",
> gai_strerror(ecode));
OK (seems to be a null change?)
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150203213023.I1264>