Date: Sat, 2 Jun 2012 18:15:40 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John Baldwin <jhb@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Eitan Adler <eadler@FreeBSD.org>, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r236377 - head/sys/dev/vxge/vxgehal Message-ID: <20120602175006.H990@besplex.bde.org> In-Reply-To: <201206011316.58330.jhb@freebsd.org> References: <201206010423.q514NKtf083043@svn.freebsd.org> <201206011024.49122.jhb@freebsd.org> <CAF6rxgnTFVBNOewiPXNvHyt3YamWsa6YZDPFtM4-uumUD6Eqdg@mail.gmail.com> <201206011316.58330.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 1 Jun 2012, John Baldwin wrote:
> On Friday, June 01, 2012 12:39:48 pm Eitan Adler wrote:
>> On 1 June 2012 07:24, John Baldwin <jhb@freebsd.org> wrote:
>>> This is why I personally loathe assignment side effects in boolean expressions
>>> for control flow. I tend to write this sort of thing instead as:
>>>
>>> channel->dtr_arr[dtr_index].dtr = dtrh;
>>> if (dtrh != NULL) {
>>
>> Same here. I was told to use the assignment is style(9) by multiple
>> people though. If it really is, I wish it would change.
>
> style(9) doesn't make a clear statement either way, but it has contradicting
> examples.
>
> First:
>
> while ((ch = getopt(argc, argv, "abNn:")) != -1)
>
> (and this one I use all the time myself as that is the common idiom for
> getopt())
>
> Second:
>
> error = function(a1, a2);
> if (error != 0)
> exit(error);
The former is the old style and is still common. We tried to change this
10-15 years ago, but in style(9) didn't change much except the second
example. In Lite2 it is:
/* No spaces after function names. */
if (error = function(a1, a2))
exit(error);
and we successfully changed almost everything/everyone to not have/write
code like that (the compiler will now warn about the test-by-assignment),
but often the fix is to just add parentheses and a test on the same line
since that minimises churn and some writers prefer it.
> Also, style(9) tends to frown on assignments that are side-effects in other
> places, for example:
>
> Be careful to not obfuscate the code by initializing variables in the
> declarations. Use this feature only thoughtfully. DO NOT use function
> calls in initializers.
This was originally not about side effects, but about writing _any_ of the
function in initializers. In Lite2, it is:
* DO NOT initialize variables in the declarations.
This rule is even more useful for minimising bugs without really trying
than it used to be, since the initializations need to be moved after a
lock in many cases, but FreeBSD weakened it as above :-(.
> struct foo one, *two;
> double three;
> int *four, five;
> char *six, seven, eight, nine, ten, eleven, twelve;
I recently discussed ordering of declarations with someone and noticed
that style(9) mostly violates its own rules by not sorting on either
size or alphabetically here. The rule should be to sort on alignment
and then alphabetically, and gives something like:
struct foo one; /* structs always first */
double three; /* big FP types next */
struct foo *two; /* then pointers (sort on complexity?) */
int *four;
char *six;
int five;
/* XXX bogus names; keep their non-alphabetical sorting below. */
char seven, eight, nine, ten, eleven, twelve;
but my rules put all scalars after all pointers (so the double goes after
the pointers despite it being larger and having stricter alignment
requirements on i386), and there are many complications for typedefed
opaque and semi-opaque types (you may or may not know anything about
their size and alignment).
> Some newer changes added at the end do use assignment side-effects while
> demonstrating other rules (the !p example and err(3) and warn(3) examples,
> this last I find particularly obfuscated and painful to read).
These are actually old. In Lite2 they are:
* ... Also, test pointers
* against NULL, i.e. use:
*
* (p = f()) == NULL
* not:
* !(p = f())
* ...
* Use err/warn(3), don't roll your own!
*/
if ((four = malloc(sizeof(struct foo))) == NULL)
err(1, NULL);
if ((six = (int *)overflow()) == NULL)
errx(1, "Number overflowed.");
For !p, it just says not to use !p for pointers, and the test in the
same expression as the assignment is as usual.
> However, I do split it out if the return value is used for more than error
> checking, e.g.:
>
> fd = open(...);
> if (fd < 0)
> err(1, "open");
The best way.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120602175006.H990>