Date: Tue, 4 Nov 2014 04:06:51 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: d@delphij.net Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, Bruce Evans <brde@optusnet.com.au>, Ian Lepore <ian@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, Mark R V Murray <mark@grondar.org>, Konstantin Belousov <kostikbel@gmail.com> Subject: Re: svn commit: r273958 - head/sys/dev/random Message-ID: <20141104025944.F1038@besplex.bde.org> In-Reply-To: <54573E07.5040505@delphij.net> References: <201411020201.sA221unt091493@svn.freebsd.org> <720EB74E-094A-43F3-8B1C-47BC7F6FECC3@grondar.org> <1414934579.17308.248.camel@revolution.hippie.lan> <6FB65828-6A79-4BDE-A9F7-BC472BA538CE@grondar.org> <CAJ-VmomeOwE3LOpehhJ__G=FCoBDRXrrn%2BSfjwPFODts6YYHNQ@mail.gmail.com> <20141102192057.GB53947@kib.kiev.ua> <29A795E1-19E2-49E4-9653-143D3F6F3F12@grondar.org> <20141102194625.GC53947@kib.kiev.ua> <751CD860-95B9-4F68-AE69-976B42823AD0@grondar.org> <54568E41.8030305@delphij.net> <20141102201331.GE53947@kib.kiev.ua> <545693B4.8030602@delphij.net> <1414961583.1200.27.camel@revolution.hippie.lan> <5456A47E.2030405@delphij.net> <5456A69C.6040902@delphij.net> <20141103115402.H3149@besplex.bde.org> <54573E07.5040505@delphij.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 3 Nov 2014, Xin Li wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> On 11/2/14 5:13 PM, Bruce Evans wrote:
>> % - % - KASSERT(random_adaptor != NULL, ("No active random
>> adaptor in %s", __func__)); % } % % void
>>
>> Lots of style bugs (long lines, use of the __func__ obfuscation,
>> and general verboseness).
>
> What would you recommend doing for cases like this? Do we just do
> KASSERT without the __func__ and rely on ddb to provide the
> information in backtrace or something else?
The implementation of KASSERT() should have been that. ddb, addr2line,
and similar programs can produce better results at negative costs to
the source and object code sizes. See an old flamew^Wdiscussion.
Similarly for lock and mutex macros. Inlining complicates things,
so that full debugging info is required to recover function names
and context. __func__ sometimes gives the name of the function that
you want, but not always. It is not useful to know the name of a
deeply nested inline function -- instead the context is usually needed.
Outisde of macros, I consider use of __func__ to be an obfuscation. It
is easier to write but harder to read and useless for grepping. It is
useful for macros like mtx_lock_spin() for the debugging case implemented
with no ddb/addr2line support, since unlike for KASSERT() the API doesn't
support passing in the caller's name. (mtx_lock_spin() actually uses the
weaker __FILE__ and stronger __LINE__, and never __func__.) It is also
rarely useful for inline functions, since there it gives the name of the
inline function but usually you want the name of the immediate caller.
If you want the name of the caller several steps up, then there is nothing
better than a backtrace that has not been broken by inlining.
assert(3) gets this wrong in the opposite direction. With assert(), the
message is generated automatically from the assertion and there is no
way to keep __FILE__, __LINE__ or __func__ out of it, though the
implementation could produce these virtually to minimize the runtime
bloat. The only way to add some extra info is to use some hack like
the comma operator to add a string literal. So the above would be
written as:
assert(random_adaptor != NULL);
which says almost as much in 1/3 the space and 1/10 of the mess in the
source code, or with a different mess:
assert(("X != NULL means no X (duh)", random_adaptor != NULL));
> Speaking for long lines -- I usually use the MTA's default so the diff
> won't wrap. style(9) is a little bit vague here, what should new code
> appear like?
>
>> % @@ -256,11 +258,15 @@ random_adaptor_read(struct cdev *dev
>> __unused, str % /* Let the entropy source do any post-read
>> cleanup. */ % (random_adaptor->ra_read)(NULL, 1); % % -
>> free(random_buf, M_ENTROPY); % + if (nbytes !=
>> uio->uio_resid && (error == ERESTART || % + error ==
>> EINTR) ) % + error = 0; /* Return partial read, not
>> error. */
I consider MTA wrapping a bug. It destroyed the diff after a couple of
rounds of quoting. Some MTAs don't like my "% " quoting for diffs.
style(9) just has an 80- or 79-column limit, stated by example of not
having any lines longer than that. FreeBSD broke this by mangling the
example C code into a man page. man adds a 5-space left margin and
also normally formats for 80 columns (or just the terminal's width?),
so it isn't clear what the example is. It looks more like a 75- or
74-column limit. That is good too since it leaves space for quoting.
>> Extra blank lines are sprinkled randomly. Mostly added, but one
>> was removed. There is an extra blank line after the sx_slock() that
>> separates it from the blocks(s) of code that it locks; the removed
>> one used the same style.
>
> Will change the code to consistently use no blank line after
> sx_slock() and before sx_sunlock() if that's correct, can you confirm?
I would probably put these blank lines back, to conform to the original
(non-KNF) style.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141104025944.F1038>