Date: Fri, 9 Mar 2018 20:50:32 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Conrad Meyer <cem@freebsd.org> Cc: Bruce Evans <brde@optusnet.com.au>, Mark Johnston <markj@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r330663 - head/sys/kern Message-ID: <20180309194900.F2122@besplex.bde.org> In-Reply-To: <CAG6CVpUE8JC5z_gHdzzHmmCuZCkPZ2tCwwQYPgr%2BszQ5hy-kBg@mail.gmail.com> References: <201803081704.w28H4aQx052056@repo.freebsd.org> <20180309150402.X950@besplex.bde.org> <CAG6CVpUE8JC5z_gHdzzHmmCuZCkPZ2tCwwQYPgr%2BszQ5hy-kBg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 8 Mar 2018, Conrad Meyer wrote:
> On Thu, Mar 8, 2018 at 8:42 PM, Bruce Evans <brde@optusnet.com.au> wrote:
>> On Thu, 8 Mar 2018, Mark Johnston wrote:
>>> ...
>>> +++ head/sys/kern/kern_shutdown.c Thu Mar 8 17:04:36 2018
>>> (r330663)
>>> @@ -1115,6 +1115,12 @@ dump_check_bounds(struct dumperinfo *di, off_t
>>> offset,
>>>
>>> if (length != 0 && (offset < di->mediaoffset ||
>>> offset - di->mediaoffset + length > di->mediasize)) {
>>> + if (di->kdcomp != NULL && offset >= di->mediaoffset) {
>>> + printf(
>>> + "Compressed dump failed to fit in device
>>> boundaries.\n");
>>> + return (E2BIG);
>>> + }
>>> +
>>
>> Style bug: extra blank line. Even the outer if statements in this function
>> are missing this error.
>
> No. Style(9) does not and should not require the complete absence of
> blank lines. Committers are free to use their best judgement on how
> to separate blocks of code. I think Mark's decision is eminently
> reasonable.
style(9) does require this. This is hard to see since the style guide
has been mangled by converting it into a man page, with markup in non
C-code and lots of blank lines to separate the markup. See
/usr/src/admin/style/style in 4.4BSD or /usr/src/share/misc/style in
NetBSD for non-mangled versions. These are pure C code which give
many rules implicitly by example. All versions give 2 or 3 rules for
leaving a blank line between groups of header files. These rules are
somewhat needed since leaving blank lines is so unusual.
It is also the default for indent(1) (indent -sob). indent removes far too
many blank lines by default. I like to have optional blank lines only
before blocks of code headed by a comment (with the blank line before the
comment). But is more common in KNF code to not have a blank line before
a block comment (since the "/*" for the first line of the comment is an
adequate separator).
Blank lines might be used to separate sections of code, but the above
blank line is a very large style bug since it separates related code,
while the style nearby is to not separate even unrelated code.
kern_shutdown.c has very few extra blank lines. indent only wants to
remove 2. I see a few more:
- the include of sys/signalvar.h is separated from all other includes
by a blank line (and more so by not sorting it into the sys includes
section)
- similarly for the include of machine/stdarg.h. This has an attached
comment about K&R support that wasn't needed even when K&R was supported.
- boot() has an extra blank line between initializing waittime() and
calling sync(). These are unrelated, but the code is so simple (just
2 statements) that splitting it up is not useful. Elsewhere, boot()
uses lots of little sections headed by comments with a block line before
the comments except for some block comments.
- panic() has a blank line before a va_start() but no blank line after the
matching va_end(), then blank line after the next statement. The second
blank line is correct for matching the first one since the next statement
is related (it completes printing a message).
- kproc_shutdown() has a bogus blank line after a return statement, and a
nonsense blank line to separate checking of 'error' from setting it.
- kthread() copies the bad style of kproc_shutdown() perfectly.
Dumper code had no extra blank lines. This is easy enough since it is small
(less than 10% of the file).
>>> printf("Attempt to write outside dump device
>>> boundaries.\n"
>>> "offset(%jd), mediaoffset(%jd), length(%ju),
>>> mediasize(%jd).\n",
>>> (intmax_t)offset, (intmax_t)di->mediaoffset,
>> ...
>>
>> Other style bugs:
>> - capitalization of error messages
>> - termination of error messages with a period
>
> Why do you think these are style bugs?
Because they are. Error messages are conventionally not capitalized or
terminated with a period. You snipped my partial explanation by example
that err(3) almost enforces this in userland by not supporting it.
Userland messages look like "foo: bar: Bad style", where 'foo' is the program
name and it would be unUnix-like to capitalize it; 'bar' might be a function
name and it would be Unix-like to capitalize that too, or it might be an
English word and then it is unclear whose style rules apply (IIRC,
capitalization after a colon only depends on style); and "Bad style" is
strerror(ESTYLE) -- for some reason, strings returned by strerror() are
always capitalized so they match some style rules for colons, and then using
different style rules for 'bar' is either a bug or a feature.
There is not much enforcement of this in the kernel. panic() is a bit
like err(). Using device_printf() gives a uniform style for the first
word in device driver messages. It would be unUnix-line to capitalize
device names, so this gives uncapitalized messages if the style rule
used for capitalization after colons is to not capitalize.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180309194900.F2122>