{"thread":{"id":"62304","subject":"[PATCH v2 1/3] clang-format: change column limit to 96 characters","startedAt":"2024-10-10T17:59:16Z","lastAt":"2024-10-10T20:09:56Z","messageCount":7,"participants":["Karthik Nayak","Kyle Lippincott","karthik nayak"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"504723","messageId":"cover.1728582927.git.karthik.188@gmail.com","threadId":"62304","inReplyTo":"CAOLa=ZRvFBhageS65uE5enzLBz7H_CAvvnEcPsi_QAi0exRx2w@mail.gmail.com","subject":"[PATCH v2 0/3] clang-format: fix rules to make the CI job cleaner","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-10-10T17:59:08Z","receivedAt":"2024-10-10T17:59:16Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The clang-format CI job is currently cluttered due to too many errors being\nreported. See some of the examples here:\n\n* https://gitlab.com/gitlab-org/git/-/jobs/7854601948\n* https://gitlab.com/gitlab-org/git/-/jobs/7843131109\n\nSo modify the clang-format with the following changes:\n1. Modify the column limit to 96 to provide some slack so we can make code more\nreadable and don't have to be constrained to 80 characters always. This is the\ncause of most of the errors reported by the tool and should cleanup the reports\nso we can actually focus on the real remaining issues.\n2. Don't align expressions after linebreaks to ensure that we instead rely on\n'ContinuationIndentWidth'. This fix is rather small and ensures that instead of\ntrying to align wrapped expressions, we follow the indentation width.\n3. Align the macro definitions. This is something we follow to keep the macros\nreadable. \n\nI will still keep monitoring the jobs from time to time to ensure we can fine\ntune more as needed, if someone see's something odd, do keep me in the loop.\n\nThanks\n\nChanges over the previous version:\n1. Change the column limit from 0 to 96, this way we still have a column limit\nbut provide some slack.\n2. Add an example for the third commit.\n\nKarthik Nayak (3):\n clang-format: change column limit to 96 characters\n clang-format: don't align expressions after linebreaks\n clang-format: align consecutive macro definitions\n\n .clang-format | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\nRange-diff against v1:\n1: 34cd0c29f6 ! 1: e22ffbe0f6 clang-format: don't enforce the column limit\n @@ Metadata\n Author: Karthik Nayak \n \n ## Commit message ##\n - clang-format: don't enforce the column limit\n + clang-format: change column limit to 96 characters\n \n The current value for the column limit is set to 80. While this is as\n expected, we often prefer readability over this strict limit. This means\n it is common to find code which extends over 80 characters. So let's\n - change the column limit to be 0 instead. This ensures that the formatter\n - doesn't complain about code strictly not following the column limit.\n + change the column limit to be 96 instead. This provides some slack so we\n + can ensure readability takes preference over the 80 character hard\n + limit.\n \n Signed-off-by: Karthik Nayak \n \n @@ .clang-format: UseTab: Always\n ContinuationIndentWidth: 8\n -ColumnLimit: 80\n +\n -+# While we recommend keeping column limit to 80, we don't want to\n -+# enforce it as we generally are more lenient with this rule and\n -+# prefer to prioritize readability.\n -+ColumnLimit: 0\n ++# While we recommend keeping column limit to 80, we want to also provide\n ++# some slack to maintain readability.\n ++ColumnLimit: 96\n \n # C Language specifics\n Language: Cpp\n2: 3557012fea = 2: b55d5d2c14 clang-format: don't align expressions after linebreaks\n3: ad023a6cf6 ! 3: 6ebcd2690e clang-format: align consecutive macro definitions\n @@ Metadata\n ## Commit message ##\n clang-format: align consecutive macro definitions\n \n - We generally align consecutive macro definitions for better readability.\n + We generally align consecutive macro definitions for better readability:\n +\n + #define OUTPUT_ANNOTATE_COMPAT (1U<<0)\n + #define OUTPUT_LONG_OBJECT_NAME (1U<<1)\n + #define OUTPUT_RAW_TIMESTAMP (1U<<2)\n + #define OUTPUT_PORCELAIN (1U<<3)\n +\n + over\n +\n + #define OUTPUT_ANNOTATE_COMPAT (1U<<0)\n + #define OUTPUT_LONG_OBJECT_NAME (1U<<1)\n + #define OUTPUT_RAW_TIMESTAMP (1U<<2)\n + #define OUTPUT_PORCELAIN (1U<<3)\n +\n So let's add the rule in clang-format to follow this.\n \n Signed-off-by: Karthik Nayak \n-- \n2.47.0\n\n"},{"id":"504722","messageId":"e22ffbe0f65971579809d817984766af12898127.1728582927.git.karthik.188@gmail.com","threadId":"62304","inReplyTo":"cover.1728582927.git.karthik.188@gmail.com","subject":"[PATCH v2 1/3] clang-format: change column limit to 96 characters","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-10-10T17:59:09Z","receivedAt":"2024-10-10T17:59:17Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The current value for the column limit is set to 80. While this is as\nexpected, we often prefer readability over this strict limit. This means\nit is common to find code which extends over 80 characters. So let's\nchange the column limit to be 96 instead. This provides some slack so we\ncan ensure readability takes preference over the 80 character hard\nlimit.\n\nSigned-off-by: Karthik Nayak \n---\n .clang-format | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/.clang-format b/.clang-format\nindex 41969eca4b..684ab32d28 100644\n--- a/.clang-format\n+++ b/.clang-format\n@@ -12,7 +12,10 @@ UseTab: Always\n TabWidth: 8\n IndentWidth: 8\n ContinuationIndentWidth: 8\n-ColumnLimit: 80\n+\n+# While we recommend keeping column limit to 80, we want to also provide\n+# some slack to maintain readability.\n+ColumnLimit: 96\n \n # C Language specifics\n Language: Cpp\n-- \n2.47.0\n\n"},{"id":"504724","messageId":"b55d5d2c14142fddad0b5bb84f41dd1c4eb6397a.1728582927.git.karthik.188@gmail.com","threadId":"62304","inReplyTo":"cover.1728582927.git.karthik.188@gmail.com","subject":"[PATCH v2 2/3] clang-format: don't align expressions after linebreaks","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-10-10T17:59:10Z","receivedAt":"2024-10-10T17:59:18Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"We enforce alignment of expressions after linebreaks. Which means for\ncode such as\n\n return a || b;\n\nit will expect:\n\n return a ||\n b;\n\nwe instead want 'b' to be indent with tabs, which is already done by the\n'ContinuationIndentWidth' variable. So let's explicitly set\n'AlignOperands' to false.\n\nSigned-off-by: Karthik Nayak \n---\n .clang-format | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/.clang-format b/.clang-format\nindex 684ab32d28..d655b08ff6 100644\n--- a/.clang-format\n+++ b/.clang-format\n@@ -42,10 +42,9 @@ AlignConsecutiveDeclarations: false\n # int cccccccc;\n AlignEscapedNewlines: Left\n \n-# Align operands of binary and ternary expressions\n-# int aaa = bbbbbbbbbbb +\n-# cccccc;\n-AlignOperands: true\n+# Don't enforce alignment after linebreaks and instead\n+# rely on the ContinuationIndentWidth value.\n+AlignOperands: false\n \n # Don't align trailing comments\n # int a; // Comment a\n-- \n2.47.0\n\n"},{"id":"504725","messageId":"6ebcd2690e05820e15584fe5459dc46011fde101.1728582927.git.karthik.188@gmail.com","threadId":"62304","inReplyTo":"cover.1728582927.git.karthik.188@gmail.com","subject":"[PATCH v2 3/3] clang-format: align consecutive macro definitions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-10-10T17:59:11Z","receivedAt":"2024-10-10T17:59:19Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"We generally align consecutive macro definitions for better readability:\n\n #define OUTPUT_ANNOTATE_COMPAT (1U<<0)\n #define OUTPUT_LONG_OBJECT_NAME (1U<<1)\n #define OUTPUT_RAW_TIMESTAMP (1U<<2)\n #define OUTPUT_PORCELAIN (1U<<3)\n\nover\n\n #define OUTPUT_ANNOTATE_COMPAT (1U<<0)\n #define OUTPUT_LONG_OBJECT_NAME (1U<<1)\n #define OUTPUT_RAW_TIMESTAMP (1U<<2)\n #define OUTPUT_PORCELAIN (1U<<3)\n\nSo let's add the rule in clang-format to follow this.\n\nSigned-off-by: Karthik Nayak \n---\n .clang-format | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/.clang-format b/.clang-format\nindex d655b08ff6..22ae882923 100644\n--- a/.clang-format\n+++ b/.clang-format\n@@ -35,6 +35,9 @@ AlignConsecutiveAssignments: false\n # double b = 3.14;\n AlignConsecutiveDeclarations: false\n \n+# Align consecutive macro definitions.\n+AlignConsecutiveMacros: true\n+\n # Align escaped newlines as far left as possible\n # #define A \\\n # int aaaa; \\\n-- \n2.47.0\n\n"},{"id":"504726","messageId":"CAO_smVjuseH6WJgU5CRg5kyZ7J1mgqLjrvxzapR0xB6XvQ7VdA@mail.gmail.com","threadId":"62304","inReplyTo":"e22ffbe0f65971579809d817984766af12898127.1728582927.git.karthik.188@gmail.com","subject":"Re: [PATCH v2 1/3] clang-format: change column limit to 96 characters","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-10-10T18:11:19Z","receivedAt":"2024-10-10T18:11:38Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Thu, Oct 10, 2024 at 11:00 AM Karthik Nayak wrote:\n>\n> The current value for the column limit is set to 80. While this is as\n> expected, we often prefer readability over this strict limit. This means\n> it is common to find code which extends over 80 characters. So let's\n> change the column limit to be 96 instead. This provides some slack so we\n> can ensure readability takes preference over the 80 character hard\n> limit.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> .clang-format | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/.clang-format b/.clang-format\n> index 41969eca4b..684ab32d28 100644\n> --- a/.clang-format\n> +++ b/.clang-format\n> @@ -12,7 +12,10 @@ UseTab: Always\n> TabWidth: 8\n> IndentWidth: 8\n> ContinuationIndentWidth: 8\n> -ColumnLimit: 80\n> +\n> +# While we recommend keeping column limit to 80, we want to also provide\n> +# some slack to maintain readability.\n> +ColumnLimit: 96\n>\n> # C Language specifics\n> Language: Cpp\n> --\n> 2.47.0\n>\n>\n\nI think this means that the next automated `clang-format` invocation\nwill un-wrap lines that were wrapped at 80 columns (not characters)\nbut fit in 96 columns. Modifying this setting and running\n`clang-format -i *.{c,h}` produces a lot of diffs of that kind. I\ndon't think there's a way of setting a soft column limit in\nclang-format.\n\nPersonally, I'd be fine with a higher column limit, but we'd need to\nmake a conscious change to the style guidelines for that.\n"},{"id":"504738","messageId":"CAOLa=ZQ=-GwnCX7CP6sqmoArphQuUNbfcCje2_64k9g8yJ1PBA@mail.gmail.com","threadId":"62304","inReplyTo":"CAO_smVjuseH6WJgU5CRg5kyZ7J1mgqLjrvxzapR0xB6XvQ7VdA@mail.gmail.com","subject":"Re: [PATCH v2 1/3] clang-format: change column limit to 96 characters","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-10-10T19:49:21Z","receivedAt":"2024-10-10T19:49:23Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Kyle Lippincott writes:\n\n> On Thu, Oct 10, 2024 at 11:00 AM Karthik Nayak wrote:\n>>\n>> The current value for the column limit is set to 80. While this is as\n>> expected, we often prefer readability over this strict limit. This means\n>> it is common to find code which extends over 80 characters. So let's\n>> change the column limit to be 96 instead. This provides some slack so we\n>> can ensure readability takes preference over the 80 character hard\n>> limit.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> .clang-format | 5 ++++-\n>> 1 file changed, 4 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/.clang-format b/.clang-format\n>> index 41969eca4b..684ab32d28 100644\n>> --- a/.clang-format\n>> +++ b/.clang-format\n>> @@ -12,7 +12,10 @@ UseTab: Always\n>> TabWidth: 8\n>> IndentWidth: 8\n>> ContinuationIndentWidth: 8\n>> -ColumnLimit: 80\n>> +\n>> +# While we recommend keeping column limit to 80, we want to also provide\n>> +# some slack to maintain readability.\n>> +ColumnLimit: 96\n>>\n>> # C Language specifics\n>> Language: Cpp\n>> --\n>> 2.47.0\n>>\n>>\n>\n> I think this means that the next automated `clang-format` invocation\n> will un-wrap lines that were wrapped at 80 columns (not characters)\n> but fit in 96 columns. Modifying this setting and running\n> `clang-format -i *.{c,h}` produces a lot of diffs of that kind. I\n> don't think there's a way of setting a soft column limit in\n> clang-format.\n\nAh! Good point.\n\n> Personally, I'd be fine with a higher column limit, but we'd need to\n> make a conscious change to the style guidelines for that.\n\nWith this, I would say that the best choice here would be to actually\nset it to 0 like the previous version. So that we don't actually enforce\nthe column limit.\n\nWe could perhaps set the value here in the '.clang-format' to 0. While\nalso setting 'max_line_length = 95' in the '.editorconfig'. That would\nmean that we don't enforce a width, but we nudge editors to wrap at 95\ncharacters. Here contributors would still have the power to decide the\nadequate width as needed.\n"},{"id":"504740","messageId":"CAO_smVgasPSWEH3UOnGo-wvKT-nE-pYmG-L8gHPCajBthGmndg@mail.gmail.com","threadId":"62304","inReplyTo":"CAOLa=ZQ=-GwnCX7CP6sqmoArphQuUNbfcCje2_64k9g8yJ1PBA@mail.gmail.com","subject":"Re: [PATCH v2 1/3] clang-format: change column limit to 96 characters","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-10-10T20:09:38Z","receivedAt":"2024-10-10T20:09:56Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Thu, Oct 10, 2024 at 12:49 PM karthik nayak wrote:\n>\n> Kyle Lippincott writes:\n>\n> > On Thu, Oct 10, 2024 at 11:00 AM Karthik Nayak wrote:\n> >>\n> >> The current value for the column limit is set to 80. While this is as\n> >> expected, we often prefer readability over this strict limit. This means\n> >> it is common to find code which extends over 80 characters. So let's\n> >> change the column limit to be 96 instead. This provides some slack so we\n> >> can ensure readability takes preference over the 80 character hard\n> >> limit.\n> >>\n> >> Signed-off-by: Karthik Nayak \n> >> ---\n> >> .clang-format | 5 ++++-\n> >> 1 file changed, 4 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/.clang-format b/.clang-format\n> >> index 41969eca4b..684ab32d28 100644\n> >> --- a/.clang-format\n> >> +++ b/.clang-format\n> >> @@ -12,7 +12,10 @@ UseTab: Always\n> >> TabWidth: 8\n> >> IndentWidth: 8\n> >> ContinuationIndentWidth: 8\n> >> -ColumnLimit: 80\n> >> +\n> >> +# While we recommend keeping column limit to 80, we want to also provide\n> >> +# some slack to maintain readability.\n> >> +ColumnLimit: 96\n> >>\n> >> # C Language specifics\n> >> Language: Cpp\n> >> --\n> >> 2.47.0\n> >>\n> >>\n> >\n> > I think this means that the next automated `clang-format` invocation\n> > will un-wrap lines that were wrapped at 80 columns (not characters)\n> > but fit in 96 columns. Modifying this setting and running\n> > `clang-format -i *.{c,h}` produces a lot of diffs of that kind. I\n> > don't think there's a way of setting a soft column limit in\n> > clang-format.\n>\n> Ah! Good point.\n>\n> > Personally, I'd be fine with a higher column limit, but we'd need to\n> > make a conscious change to the style guidelines for that.\n>\n> With this, I would say that the best choice here would be to actually\n> set it to 0 like the previous version. So that we don't actually enforce\n> the column limit.\n>\n> We could perhaps set the value here in the '.clang-format' to 0. While\n> also setting 'max_line_length = 95' in the '.editorconfig'. That would\n> mean that we don't enforce a width, but we nudge editors to wrap at 95\n> characters. Here contributors would still have the power to decide the\n> adequate width as needed.\n\nOne thing to be cautious of is column vs character distinctions.\nBecause we use tabs[^1], one character regularly has a display width\nof 8 columns.\n\n[^1]: Another personal opinion, and this is going to be very\ncontentious so please don't think that others agree with me on this,\nbecause I suspect I'm in the minority: tabs are a massive mistake. The\n*only* benefit we get from our current use of tabs is smaller source\nfiles, and we get numerous headaches because of them. I could go on\nabout this, because I have Opinions here, but this probably isn't the\nright place ;)\n"}]}