Page MenuHomeFreeBSD

Update style.9 to reflect consensus on developer's mailing list.
ClosedPublic

Authored by imp on Jun 16 2015, 9:09 PM.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
imp marked 2 inline comments as done.Jun 17 2015, 3:14 AM
imp added inline comments.
share/man/man9/style.9
506 ↗(On Diff #6249)

This is better, but still too wordy. I don't like it any better than what we have now.

513 ↗(On Diff #6249)

But Ian's wording changes the meaning of what we have there now. While it is simpler, it is less clear as a result.

571 ↗(On Diff #6249)

gnn's wording is still too verbose, but it was a comment against a much earlier draft.

imp updated this revision to Diff 6256.Jun 17 2015, 3:23 AM
imp edited edge metadata.

Blend together the different suggestsions into a less verbose, clear
and concise recasting.

Eliminate changes later in the file. They are redundant and don't
really go with what was originally being said there when read in the
larger context.

This revision now requires review to proceed.Jun 17 2015, 3:23 AM
imp marked 8 inline comments as done.Jun 17 2015, 3:25 AM

Updated with better verbage culled from all the suggestions, even the ones I didn't quite like since they also had elements that I could work with.

share/man/man9/style.9
569 ↗(On Diff #6256)

Also, it turns out that the changes in this part of the file aren't needed at all, so I reverted back to the original.

imp marked 2 inline comments as done.Jun 17 2015, 3:26 AM

finsh

ian added inline comments.Jun 17 2015, 3:26 AM
share/man/man9/style.9
513 ↗(On Diff #6249)

In what way does it change the meaning? Either you consistantly always use braces, or you consistantly only use them to enclose multiple lines (leaving single lines braceless).

imp added inline comments.Jun 17 2015, 3:28 AM
share/man/man9/style.9
513 ↗(On Diff #6256)

you left off the 'except for clarity' or 'when things span multiple lines' or some variation. That bit is important.

araujo accepted this revision.Jun 17 2015, 3:34 AM
araujo edited edge metadata.

I dislike this part: +Usage within a file should be consistent.

It means, I need now be more careful with two different style just to keep consistent the code. I still prefer no braces for single line.

I think we should relax the usage of braces in single line statement, but not enforce it as a must. For me the style(9) is a guide, and not a must, lots of code in our base system doesn't follow it 100%.

This revision is now accepted and ready to land.Jun 17 2015, 3:34 AM
allanjude accepted this revision.Jun 17 2015, 3:35 AM
allanjude edited edge metadata.
julian added a subscriber: julian.Jun 17 2015, 3:44 AM
julian accepted this revision.Jun 17 2015, 3:53 AM
julian added a reviewer: julian.

I don't mind if people leave them out but I do prefer to see them in, especially if there are other clauses. This is true in if, for, while, etc.... gratuitously removing them makes the single line clauses not match the multiple line clauses in style.
I really hate if-elseif-elseif-elseif-else chains where some clauses have them, and some do not.

share/man/man9/style.9
513 ↗(On Diff #6256)

I just think removal of the requirement to NOT have them is enough. The wording here seems clunky.

ed accepted this revision.Jun 17 2015, 4:00 AM
ed added a reviewer: ed.
deischen added inline comments.Jun 17 2015, 4:05 AM
share/man/man9/style.9
513 ↗(On Diff #6256)

I don't like having to use them consistently in the same file, for a point brought up by julian. In nested conditionals, as in if, else-if, else-if, it may be aesthetically pleasing to see the braces all match, but yet not have to have braces for simple single statement conditions elsewhere in the file.

imp marked 3 inline comments as done.Jun 17 2015, 4:30 AM
imp added inline comments.
share/man/man9/style.9
513 ↗(On Diff #6256)

Re all the noise: "Should" not "Must". We strongly encourage style within a file to be consistent elsewhere, and this just reinforces it. You should do this, but if there's a good reason not to do it, then "should" covers that.

imp marked 2 inline comments as done.Jun 17 2015, 4:30 AM

stupid phabric

jmg added a subscriber: jmg.Jun 17 2015, 5:21 AM

needs work... style(9) is a guide, clearly not stricktly enforced, and if code is truely confusing w/o the braces, then add them, but changing style(9) is unneeded... The original example of double goto fail doesn't point out that:
if (case) {

goto fail;

}
goto fail;

would have probably just as likely slipped through the code review, or any other variation.

Looks good.

BTW: I think this is the current style.

--HPS

rrs added a subscriber: rrs.Jun 17 2015, 8:00 AM
mat accepted this revision.Jun 17 2015, 9:40 AM
mat added a reviewer: mat.
mat added a subscriber: mat.

Macro this-is-freebsd:

imp added a comment.Jun 17 2015, 3:12 PM
In D2842#54831, @jmg wrote:

needs work... style(9) is a guide, clearly not stricktly enforced, and if code is truely confusing w/o the braces, then add them, but changing style(9) is unneeded... The original example of double goto fail doesn't point out that:
if (case) {

goto fail;

}
goto fail;

would have probably just as likely slipped through the code review, or any other variation.

The ask is to relax the guide to reflect the increasingly common practice and desire of always including them. The guide already allows for them to make things clearer (though said in the most opaque way possible). The change isn't to allow them when they make things clearer (though the wording to make that explicit rather than this tortured overly specific exception is in there). Some people want to always have them in their code, even if it isn't needed to make things clearer. While personally I hate that style (takes up too much vertical space), it's the desire of a large number of people that have spoken.

Still think the change unnecessary?

rrs abandoned this revision.Jun 17 2015, 3:24 PM
In D2842#54831, @jmg wrote:

needs work... style(9) is a guide, clearly not stricktly enforced, and if code is truely confusing w/o the braces, then add them, but changing style(9) is unneeded... The original example of double goto fail doesn't point out that:
if (case) {

goto fail;

}
goto fail;

would have probably just as likely slipped through the code review, or any other variation.

It might have slipped through code review, but it's less likely to have been introduced in the first place if braces are used.

David Wheeler's got a pretty good description: http://www.dwheeler.com/essays/apple-goto-fail.html

imp reclaimed this revision.Jun 17 2015, 3:27 PM

Phabricator hates large number of reviewers?

This revision is now accepted and ready to land.Jun 17 2015, 3:27 PM
gahr accepted this revision.Jun 17 2015, 4:31 PM
gahr edited edge metadata.
gahr added a reviewer: gahr.
gahr edited edge metadata.
wblock accepted this revision.Jun 17 2015, 4:45 PM
wblock added a reviewer: wblock.
wblock added inline comments.
share/man/man9/style.9
510 ↗(On Diff #6256)

I would move "allowed" earlier ("two styles of braces ({ }) are allowed for single line statements"). Otherwise, looks okay.

gad added inline comments.Jun 17 2015, 4:46 PM
share/man/man9/style.9
513 ↗(On Diff #6256)

I'm in favor of any update which relaxes the rules on this, as I do think there are times when it's better to have braces around single-line statements.

I'd like to see the rules relaxed a bit more to say that in the case of this braces-rule, usage should be consistent with a subroutine, instead of the entire source file. I do prefer file-wide consistency for most rules. But in thinking over code that I've written, there are some subroutines where it helps me to have the extra braces when I'm working on that routine. (The braces help when I'm constantly adding/removing statements for debugging purposes).

And yes, the main reason I'm saying this is just to make sure I understand how to comment on changes in Phabricator.

acm accepted this revision.Jun 17 2015, 5:01 PM
acm added a reviewer: acm.
gad added inline comments.Jun 17 2015, 5:23 PM
share/man/man9/style.9
510 ↗(On Diff #6256)

I do think it reads better to move "are allowed" sooner in the sentence.

markm accepted this revision.Jun 17 2015, 6:28 PM
markm added a reviewer: markm.
smh accepted this revision.Jun 17 2015, 7:14 PM
smh added a reviewer: smh.
rodrigc accepted this revision.Jun 17 2015, 8:34 PM
rodrigc added a reviewer: rodrigc.
imp updated this revision to Diff 6275.Jun 17 2015, 8:52 PM
imp edited edge metadata.

update based on wblock's good suggestion.

This revision now requires review to proceed.Jun 17 2015, 8:52 PM
andrew accepted this revision.Jun 17 2015, 8:57 PM
andrew edited edge metadata.
This revision is now accepted and ready to land.Jun 17 2015, 8:57 PM
This revision was automatically updated to reflect the committed changes.
vangyzen reopened this revision.Jun 18 2015, 1:10 PM
This revision is now accepted and ready to land.Jun 18 2015, 1:10 PM

Reopening the review so voting can continue...

jmg added a comment.Jun 19 2015, 3:11 PM
In D2842#54946, @emaste wrote:

It might have slipped through code review, but it's less likely to have been introduced in the first place if braces are used.

David Wheeler's got a pretty good description: http://www.dwheeler.com/essays/apple-goto-fail.html

Read it, completely unconvinced that adding braces would have caught it, anyone who missed the strange indentation of two goto statements would have missed what extra braces ment... style(9) is there to help make code easier to read, but does not alleviate the developer from reading and understanding the code...

jmg added a comment.Jun 19 2015, 3:13 PM

I would recommend a statement that adds that the preferred/recommended is no extra braces if this must go in...

In D2842#55479, @jmg wrote:

Read it, completely unconvinced that adding braces would have caught it, anyone who missed the strange indentation of two goto statements would have missed what extra braces ment... style(9) is there to help make code easier to read, but does not alleviate the developer from reading and understanding the code...

I think you've missed my point: in this case the braces aren't there to make it more likely someone would catch the extra goto fail in code review, but to make it less likely that a merge process ends up producing it at all.

jmg added a comment.Jun 19 2015, 3:29 PM
In D2842#55489, @emaste wrote:
In D2842#55479, @jmg wrote:

Read it, completely unconvinced that adding braces would have caught it, anyone who missed the strange indentation of two goto statements would have missed what extra braces ment... style(9) is there to help make code easier to read, but does not alleviate the developer from reading and understanding the code...

I think you've missed my point: in this case the braces aren't there to make it more likely someone would catch the extra goto fail in code review, but to make it less likely that a merge process ends up producing it at all.

If you're merging code and blindly accepting the merged code, then you shouldn't be merging it. Even a basic attempt to understand the code would have caught it.

Also, don't forget that was *security* code (well, all code is security code, but that's another topic), if you can't be careful w/ the code/merge, you need a new line of work.

bdrewery accepted this revision.Jun 19 2015, 7:20 PM
bdrewery added a reviewer: bdrewery.
bdrewery added a subscriber: bdrewery.

I've seen the lack of braces cause a kernel bug within the past month. Please let's just loosen this and move on.

Is there a need to define the word clarity, maybe by giving some examples? It might be subjective what is clear.

roberto accepted this revision.Jun 21 2015, 10:37 PM
roberto added a reviewer: roberto.
des accepted this revision.Jun 30 2015, 12:16 PM
des added a reviewer: des.
cy added a subscriber: cy.Jul 14 2015, 4:56 AM
cy added a reviewer: cy.Jul 14 2015, 5:01 AM
cy accepted this revision.Jul 14 2015, 5:05 AM
cy edited edge metadata.

La la la.

head/share/man/man9/style.9
510

single-line should be hyphenated when used as a compound adjective.

513

This text would be more clear if written as:

Usage within a file should be consistent: braces should either be used for all single-line statements, or only where needed for clarity.

imp closed this revision.Jul 17 2015, 10:32 PM

comitted.