| Message ID | RFC-patch-3.4-06d4b76a364-20220711T110019Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | make headway towards a clean "clang-format" | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > As with the preceding change what this leaves us with an unresolved > question though, should we have some stricter version of "make > style-all" that incorporates "ColumnLimit: 80", or perhaps apply it > only on "make style", but then what if someone modifies code that > happens to e.g. search/replace a line running afoul of the limit? A more important thing to think about is that there is no single good cut-off point. When we say "wrap your lines at around 80 columns", we mean that when there is a good place to fold at around column 65 and the next good place is at column 82, then it is OK to go slightly over 80 and wrap at 82, which may be better than wrapping at 65. If the last good place to wrap is at column 72 and the long function call at the end of the line makes you go past the 82nd column, wrapping at column 72 might be better. I wonder if there is an automated formatter that understands this kind of shades of gray and lets us express that. Thanks.
On 2022-07-11 at 11:37:27, Ævar Arnfjörð Bjarmason wrote: > Our Documentation/CodingGuidelines mention that "We try to keep to at > most 80 characters per line", but in reality we have a lot of code > that runs afoul of that rule. > > Before & after this change running "make style-all-diff-apply" will > yield: > > 579 files changed, 32065 insertions(+), 29818 deletions(-) > 509 files changed, 13042 insertions(+), 12745 deletions(-) > > As with the preceding change what this leaves us with an unresolved > question though, should we have some stricter version of "make > style-all" that incorporates "ColumnLimit: 80", or perhaps apply it > only on "make style", but then what if someone modifies code that > happens to e.g. search/replace a line running afoul of the limit? > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> As mentioned upthread, I am fine with an 80-character limit. It's a reasonable choice and what's we've traditionally done. However, I don't think we should drop a limit altogether unless we're going to not bother people about this in code review. I would say that if people are going to want a limit on line length, then we should pick one. Now, we could well pick one that's longer than 80 characters. 132 is a common terminal size and it would avoid needing to rewrap all of those lines. But sticking with 80 columns is also fine, and we'll just need to send some patches accordingly.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > Now, we could well pick one that's longer than 80 characters. 132 is a > common terminal size and it would avoid needing to rewrap all of those > lines. But sticking with 80 columns is also fine, and we'll just need > to send some patches accordingly. As long as people do not start sending an overly wide code that consistently are 130 columns wide, I am fine. Let's not encourage people to use automation as excuses for sending unreadable mess and (worse yet) push back reviewer comments when such issues that cannot be corrected with automation are pointed out. Thanks.
On Mon, Jul 11, 2022 at 6:50 PM Junio C Hamano <gitster@pobox.com> wrote: > "brian m. carlson" <sandals@crustytoothpaste.net> writes: > > Now, we could well pick one that's longer than 80 characters. 132 is a > > common terminal size and it would avoid needing to rewrap all of those > > lines. But sticking with 80 columns is also fine, and we'll just need > > to send some patches accordingly. > > As long as people do not start sending an overly wide code that > consistently are 130 columns wide, I am fine. > > Let's not encourage people to use automation as excuses for sending > unreadable mess and (worse yet) push back reviewer comments when > such issues that cannot be corrected with automation are pointed > out. In my experience, clang-format will reflow a line of code so that it fills the configured line-length. So, even if you manually wrap the code to fit nicely in 80 columns, if clang-format is set to 132 columns, then it will automatically reflow your nicely hand-wrapped 80-column code out to 132 columns, which I think is not what we'd want (at least those of us who always work in 80-column terminals and editors). But perhaps there is a configuration knob which disables clang-format's "reflow-to-occupy-full-width" behavior? brian?
Eric Sunshine <sunshine@sunshineco.com> writes: > So, even if you manually wrap the code to fit nicely in 80 > columns, if clang-format is set to 132 columns, then it will > automatically reflow your nicely hand-wrapped 80-column code out > to 132 columns, which I think is not what we'd want (at least > those of us who always work in 80-column terminals and > editors). But perhaps there is a configuration knob which disables > clang-format's "reflow-to-occupy-full-width" behavior? brian? Agreed. We should keep in mind that there is no single good fill-column. When we say "wrap your lines at around 80 columns", we mean that when there is a good place to fold at around column 65 and the next good place is at column 82, then it is OK to go slightly over 80 and wrap at 82, which may be better than wrapping at 65. If the last good place to wrap is at column 72 and the long function call at the end of the line makes you go past the 82nd column, wrapping at column 72 might be better. I wonder if there is an automated formatter that understands this kind of shades of gray and lets us express that. Thanks.
On Mon, Jul 11 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> As with the preceding change what this leaves us with an unresolved >> question though, should we have some stricter version of "make >> style-all" that incorporates "ColumnLimit: 80", or perhaps apply it >> only on "make style", but then what if someone modifies code that >> happens to e.g. search/replace a line running afoul of the limit? > > A more important thing to think about is that there is no single > good cut-off point. When we say "wrap your lines at around 80 > columns", we mean that when there is a good place to fold at around > column 65 and the next good place is at column 82, then it is OK to > go slightly over 80 and wrap at 82, which may be better than > wrapping at 65. If the last good place to wrap is at column 72 and > the long function call at the end of the line makes you go past the > 82nd column, wrapping at column 72 might be better. There's the story of the sufficiently smart compiler, and now the sufficiently smart formatter :) The proposed solution here is to punt on it, which I think makes sense if we're trying to push forward clang-format. (Which I'm really not, this RFC is something I thought I'd send in response to brian's proposal, as I'd poked at this locally a while ago, after wondering if I could make use of it myself, and whether our .clang-format was misconfigured[1]). > I wonder if > there is an automated formatter that understands this kind of shades > of gray and lets us express that. I don't think so, and setting the configuration to "0" is only a stop-gap, after all we'd still like it to wrap e.g. lines of a length of 150 or whatever, if it finds them somewhere. 1. I think after I found that some odd styling from one of Han-Wen's patches was the result of running clang-format, cf.: https://lore.kernel.org/git/CAFQ2z_PAqW+RS2Znaf2wwOJfdNfkjP1VV84=xaPu_1EAuX+u5w@mail.gmail.com/
diff --git a/.clang-format b/.clang-format index 87398d24d4f..5a106d959be 100644 --- a/.clang-format +++ b/.clang-format @@ -12,7 +12,7 @@ UseTab: Always TabWidth: 8 IndentWidth: 8 ContinuationIndentWidth: 8 -ColumnLimit: 80 +ColumnLimit: 0 # C Language specifics Language: Cpp
Our Documentation/CodingGuidelines mention that "We try to keep to at most 80 characters per line", but in reality we have a lot of code that runs afoul of that rule. Before & after this change running "make style-all-diff-apply" will yield: 579 files changed, 32065 insertions(+), 29818 deletions(-) 509 files changed, 13042 insertions(+), 12745 deletions(-) As with the preceding change what this leaves us with an unresolved question though, should we have some stricter version of "make style-all" that incorporates "ColumnLimit: 80", or perhaps apply it only on "make style", but then what if someone modifies code that happens to e.g. search/replace a line running afoul of the limit? Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- .clang-format | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)