| Message ID | e22ffbe0f65971579809d817984766af12898127.1728582927.git.karthik.188@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clang-format: fix rules to make the CI job cleaner | expand |
On Thu, Oct 10, 2024 at 11:00 AM Karthik Nayak <karthik.188@gmail.com> wrote: > > The current value for the column limit is set to 80. While this is as > expected, we often prefer readability over this strict limit. This means > it is common to find code which extends over 80 characters. So let's > change the column limit to be 96 instead. This provides some slack so we > can ensure readability takes preference over the 80 character hard > limit. > > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > --- > .clang-format | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/.clang-format b/.clang-format > index 41969eca4b..684ab32d28 100644 > --- a/.clang-format > +++ b/.clang-format > @@ -12,7 +12,10 @@ UseTab: Always > TabWidth: 8 > IndentWidth: 8 > ContinuationIndentWidth: 8 > -ColumnLimit: 80 > + > +# While we recommend keeping column limit to 80, we want to also provide > +# some slack to maintain readability. > +ColumnLimit: 96 > > # C Language specifics > Language: Cpp > -- > 2.47.0 > > I think this means that the next automated `clang-format` invocation will un-wrap lines that were wrapped at 80 columns (not characters) but fit in 96 columns. Modifying this setting and running `clang-format -i *.{c,h}` produces a lot of diffs of that kind. I don't think there's a way of setting a soft column limit in clang-format. Personally, I'd be fine with a higher column limit, but we'd need to make a conscious change to the style guidelines for that.
Kyle Lippincott <spectral@google.com> writes: > On Thu, Oct 10, 2024 at 11:00 AM Karthik Nayak <karthik.188@gmail.com> wrote: >> >> The current value for the column limit is set to 80. While this is as >> expected, we often prefer readability over this strict limit. This means >> it is common to find code which extends over 80 characters. So let's >> change the column limit to be 96 instead. This provides some slack so we >> can ensure readability takes preference over the 80 character hard >> limit. >> >> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> >> --- >> .clang-format | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/.clang-format b/.clang-format >> index 41969eca4b..684ab32d28 100644 >> --- a/.clang-format >> +++ b/.clang-format >> @@ -12,7 +12,10 @@ UseTab: Always >> TabWidth: 8 >> IndentWidth: 8 >> ContinuationIndentWidth: 8 >> -ColumnLimit: 80 >> + >> +# While we recommend keeping column limit to 80, we want to also provide >> +# some slack to maintain readability. >> +ColumnLimit: 96 >> >> # C Language specifics >> Language: Cpp >> -- >> 2.47.0 >> >> > > I think this means that the next automated `clang-format` invocation > will un-wrap lines that were wrapped at 80 columns (not characters) > but fit in 96 columns. Modifying this setting and running > `clang-format -i *.{c,h}` produces a lot of diffs of that kind. I > don't think there's a way of setting a soft column limit in > clang-format. Ah! Good point. > Personally, I'd be fine with a higher column limit, but we'd need to > make a conscious change to the style guidelines for that. With this, I would say that the best choice here would be to actually set it to 0 like the previous version. So that we don't actually enforce the column limit. We could perhaps set the value here in the '.clang-format' to 0. While also setting 'max_line_length = 95' in the '.editorconfig'. That would mean that we don't enforce a width, but we nudge editors to wrap at 95 characters. Here contributors would still have the power to decide the adequate width as needed.
On Thu, Oct 10, 2024 at 12:49 PM karthik nayak <karthik.188@gmail.com> wrote: > > Kyle Lippincott <spectral@google.com> writes: > > > On Thu, Oct 10, 2024 at 11:00 AM Karthik Nayak <karthik.188@gmail.com> wrote: > >> > >> The current value for the column limit is set to 80. While this is as > >> expected, we often prefer readability over this strict limit. This means > >> it is common to find code which extends over 80 characters. So let's > >> change the column limit to be 96 instead. This provides some slack so we > >> can ensure readability takes preference over the 80 character hard > >> limit. > >> > >> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > >> --- > >> .clang-format | 5 ++++- > >> 1 file changed, 4 insertions(+), 1 deletion(-) > >> > >> diff --git a/.clang-format b/.clang-format > >> index 41969eca4b..684ab32d28 100644 > >> --- a/.clang-format > >> +++ b/.clang-format > >> @@ -12,7 +12,10 @@ UseTab: Always > >> TabWidth: 8 > >> IndentWidth: 8 > >> ContinuationIndentWidth: 8 > >> -ColumnLimit: 80 > >> + > >> +# While we recommend keeping column limit to 80, we want to also provide > >> +# some slack to maintain readability. > >> +ColumnLimit: 96 > >> > >> # C Language specifics > >> Language: Cpp > >> -- > >> 2.47.0 > >> > >> > > > > I think this means that the next automated `clang-format` invocation > > will un-wrap lines that were wrapped at 80 columns (not characters) > > but fit in 96 columns. Modifying this setting and running > > `clang-format -i *.{c,h}` produces a lot of diffs of that kind. I > > don't think there's a way of setting a soft column limit in > > clang-format. > > Ah! Good point. > > > Personally, I'd be fine with a higher column limit, but we'd need to > > make a conscious change to the style guidelines for that. > > With this, I would say that the best choice here would be to actually > set it to 0 like the previous version. So that we don't actually enforce > the column limit. > > We could perhaps set the value here in the '.clang-format' to 0. While > also setting 'max_line_length = 95' in the '.editorconfig'. That would > mean that we don't enforce a width, but we nudge editors to wrap at 95 > characters. Here contributors would still have the power to decide the > adequate width as needed. One thing to be cautious of is column vs character distinctions. Because we use tabs[^1], one character regularly has a display width of 8 columns. [^1]: Another personal opinion, and this is going to be very contentious so please don't think that others agree with me on this, because I suspect I'm in the minority: tabs are a massive mistake. The *only* benefit we get from our current use of tabs is smaller source files, and we get numerous headaches because of them. I could go on about this, because I have Opinions here, but this probably isn't the right place ;)
diff --git a/.clang-format b/.clang-format index 41969eca4b..684ab32d28 100644 --- a/.clang-format +++ b/.clang-format @@ -12,7 +12,10 @@ UseTab: Always TabWidth: 8 IndentWidth: 8 ContinuationIndentWidth: 8 -ColumnLimit: 80 + +# While we recommend keeping column limit to 80, we want to also provide +# some slack to maintain readability. +ColumnLimit: 96 # C Language specifics Language: Cpp
The current value for the column limit is set to 80. While this is as expected, we often prefer readability over this strict limit. This means it is common to find code which extends over 80 characters. So let's change the column limit to be 96 instead. This provides some slack so we can ensure readability takes preference over the 80 character hard limit. Signed-off-by: Karthik Nayak <karthik.188@gmail.com> --- .clang-format | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)