Page MenuHomeFreeBSD

style.9: Codify tolerance for eliding blank lines
ClosedPublic

Authored by cem on May 28 2019, 5:36 PM.

Details

Summary

Consensus seems to be that eliding blank lines is acceptable for small
functions with no local variables. Codify that explicitly in the style
document.

(While here, correct usage's definition to match its declaration and
standard C.)

Test Plan

Straw poll for committers at https://reviews.freebsd.org/V8 .

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cem created this revision.May 28 2019, 5:36 PM
cem edited the test plan for this revision. (Show Details)May 28 2019, 5:45 PM

I would even be fine with "If the function has no local variables", I do not think the length should come into play at all. Can someone explain why we have this special case at all? I know we want a blank line between variable declarations and the start of the function, is this just an attempt to preserve that empty line when there are no declarations?

rgrimes accepted this revision.May 28 2019, 6:04 PM
This revision is now accepted and ready to land.May 28 2019, 6:04 PM
cem added a comment.May 28 2019, 6:09 PM

I would even be fine with "If the function has no local variables",

I would be fine with that too, but I think that might be more contentious than this weaker change. If the consensus favors permitting leaving out a blank line entirely, I'm happy to make that change.

Can someone explain why we have this special case at all?

It dates back to phk's initial adaption of BSD 4.4-Lite2 src/admin/style/style in r12823. I did not dig back into the CSRG repo.

delphij accepted this revision.May 28 2019, 6:19 PM
kib added a subscriber: kib.May 28 2019, 6:20 PM
In D20448#441421, @cem wrote:

I would even be fine with "If the function has no local variables",

I would be fine with that too, but I think that might be more contentious than this weaker change. If the consensus favors permitting leaving out a blank line entirely, I'm happy to make that change.

Can someone explain why we have this special case at all?

It dates back to phk's initial adaption of BSD 4.4-Lite2 src/admin/style/style in r12823. I did not dig back into the CSRG repo.

It is sort of logical, because when you add a local var to such function, you get a diff with only plus lines.

But I do prefer to remove the requirement for the blank line after '{, regardless of the function size.

imp accepted this revision.May 28 2019, 6:20 PM
imp added a subscriber: imp.

I'd prefer a more vague requirement, but am open to alternative statements to what I proposed.

share/man/man9/style.9
786 ↗(On Diff #58003)

I'd replace this line with 'short.'

vangyzen accepted this revision.May 28 2019, 6:47 PM
vangyzen added a subscriber: vangyzen.
vangyzen added inline comments.
share/man/man9/style.9
786 ↗(On Diff #58003)

I like what @imp said, but I would go one step farther and remove the length clause entirely.

"...no local variables."

emaste added a subscriber: emaste.May 28 2019, 7:19 PM
emaste added inline comments.
share/man/man9/style.9
780 ↗(On Diff #58003)

IMO you should commit this bug fix now

786 ↗(On Diff #58003)

+1

imp added a comment.May 28 2019, 11:46 PM

The original file can be found here:

https://github.com/weiss/original-bsd/blob/master/admin/style/style

The original version (1.1) of this file from May 20, 1991 didn't have the empty line requirement, version 1.3, also made that same day, added it.

The very first version had:

f3()
{
					/* Empty line if no variables. */
	return (1);
}

The very first version had:

f3()
{
					/* Empty line if no variables. */
	return (1);
}

Quote above As Ed beat me to the comment, I had be saying: Actually look closer at that 1.1 version, at line 119 it existed, and then in 1.3 was modified at line 159 became 194.
We can ask Kirk for clarity?

imp added a comment.May 29 2019, 12:35 AM

The very first version had:

f3()
{
					/* Empty line if no variables. */
	return (1);
}

Quote above As Ed beat me to the comment, I had be saying: Actually look closer at that 1.1 version, at line 119 it existed, and then in 1.3 was modified at line 159 became 194.
We can ask Kirk for clarity?

Nah, no need. It was the same day. He and bostic were sharing the same offices...

I had hoped to make an argument that we can drop it entirely since it wasn't original, but I see that argument fails because it wasn't true. :)

V7 unix didn't have such a file. It was in /adm/style/style, which didn't exist in V7. The phrase 'Kernel Normal Form' doesn't exist in the V7 tapes that I have access to. And the C reference manual isn't included, which is where I think the phrase originated. My original K&R is in a box somewhere, but I have no memory of there being a style section to it. The style just was the style.

imp added a comment.EditedMay 29 2019, 12:41 AM
In D20448#441532, @imp wrote:

But looking at sys/sys/alloc.c from V7 shows the 'empty line' convention (in badblock()). and suser() in fio.c. So it's an old practice :) In fact, all the places I could find in sys/sys/*.c were like this, except for fakemx.c, which is clearly just some hastily hacked out shims for something. Upon closer inspection, there's a few instances, like read and write in sys2.c that were one or two liners that omitted the blank space, but any substantial function w/o local variables had the blank line.

Then again, V7 unix didn't observe a space between if, while, and for keywords and the following (, but the style used in the K&R 1st edition did. History is funny.

cem updated this revision to Diff 58054.May 29 2019, 8:44 PM
cem marked 3 inline comments as done.

Consensus seems to be empty (blank) lines for functions with no locals should
be fully optional. Update the text to match.

Use "blank" instead of "empty", suggested by bde@, as it seems more
consistently used to describe such lines in the rest of the document.

This revision now requires review to proceed.May 29 2019, 8:44 PM
rgrimes accepted this revision.May 29 2019, 9:26 PM
This revision is now accepted and ready to land.May 29 2019, 9:26 PM
This revision was automatically updated to reflect the committed changes.