Details
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage
Event Timeline
| share/man/man9/style.9 | ||
|---|---|---|
| 506 | This is better, but still too wordy. I don't like it any better than what we have now. | |
| 513 | But Ian's wording changes the meaning of what we have there now. While it is simpler, it is less clear as a result. | |
| 569 | gnn's wording is still too verbose, but it was a comment against a much earlier draft. | |
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.
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 | 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. | |
| share/man/man9/style.9 | ||
|---|---|---|
| 513 | 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). | |
| share/man/man9/style.9 | ||
|---|---|---|
| 513 | you left off the 'except for clarity' or 'when things span multiple lines' or some variation. That bit is important. | |
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%.
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 | I just think removal of the requirement to NOT have them is enough. The wording here seems clunky. | |
| share/man/man9/style.9 | ||
|---|---|---|
| 513 | 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. | |
| share/man/man9/style.9 | ||
|---|---|---|
| 513 | 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. | |
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?
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
| share/man/man9/style.9 | ||
|---|---|---|
| 510–513 | I would move "allowed" earlier ("two styles of braces ({ }) are allowed for single line statements"). Otherwise, looks okay. | |
| share/man/man9/style.9 | ||
|---|---|---|
| 513 | 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. | |
| share/man/man9/style.9 | ||
|---|---|---|
| 510–513 | I do think it reads better to move "are allowed" sooner in the sentence. | |
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 would recommend a statement that adds that the preferred/recommended is no extra braces if this must go in...
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.
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.
La la la.
| head/share/man/man9/style.9 | ||
|---|---|---|
| 510 ↗ | (On Diff #6276) | single-line should be hyphenated when used as a compound adjective. |
| 513 ↗ | (On Diff #6276) | 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. |