[v3,0/3] clang-format: fix rules to make the CI job cleaner

Message

karthik nayak Oct. 12, 2024, 1:49 a.m. UTC

The clang-format CI job is currently cluttered due to too many errors being
reported. See some of the examples here:

* https://gitlab.com/gitlab-org/git/-/jobs/7854601948
* https://gitlab.com/gitlab-org/git/-/jobs/7843131109

So modify the clang-format with the following changes:
1. Modify the penalties for linebreaks to be more considerate towards
readability. The commit goes into detail explaining how/why.  
2. Don't align expressions after linebreaks to ensure that we instead rely on
'ContinuationIndentWidth'. This fix is rather small and ensures that instead of
trying to align wrapped expressions, we follow the indentation width.
3. Align the macro definitions. This is something we follow to keep the macros
readable. 

I will still keep monitoring the jobs from time to time to ensure we can fine
tune more as needed, if someone see's something odd, do keep me in the loop.

Thanks

Changes over the previous version:
1. I figured that we can keep the column limit to the previous 80 while still
having some breathing room by tweaking the penalties associated with line breaks.
Also CC'in Johannes here, since this builds on top of his changes.

Karthik Nayak (3):
  clang-format: re-adjust line break penalties
  clang-format: align consecutive macro definitions
  clang-format: don't align expressions after linebreaks

 .clang-format | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Range-diff against v2:
1:  e22ffbe0f6 ! 1:  74bbd2f9db clang-format: change column limit to 96 characters
    @@ Metadata
     Author: Karthik Nayak <[e-mail]>
     
      ## Commit message ##
    -    clang-format: change column limit to 96 characters
    +    clang-format: re-adjust line break penalties
     
    -    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.
    +    In 42efde4c29 (clang-format: adjust line break penalties, 2017-09-29) we
    +    adjusted the line break penalties to really fine tune what we care about
    +    while doing line breaks. Modify some of those to be more inline with
    +    what we care about in the Git project now.
    +
    +    We need to understand that the values set to penalties in
    +    '.clang-format' are relative to each other and do not hold any absolute
    +    value. The penalty arguments take an 'Unsigned' value, so we have some
    +    liberty over the values we can set.
    +
    +    First, in that commit, we decided, that under no circumstances do we
    +    want to exceed 80 characters. This seems a bit too strict. We do
    +    overshoot this limit from time to time to prioritize readability. So
    +    let's reduce the value for 'PenaltyExcessCharacter' to 10. This means we
    +    that we add a penalty of 10 for each character that exceeds the column
    +    limit. By itself this is enough to restrict to column limit. Tuning
    +    other penalties in relation to this is what is important.
    +
    +    The penalty `PenaltyBreakAssignment` talks about the penalty for
    +    breaking an assignment operator on to the next line. In our project, we
    +    are okay with this, so giving a value of 5, which is below the value for
    +    'PenaltyExcessCharacter' ensures that in the end, even 1 character over
    +    the column limit is not worth keeping an assignment on the same line.
    +
    +    Similarly set the penalty for breaking before the first call parameter
    +    'PenaltyBreakBeforeFirstCallParameter' and the penalty for breaking
    +    comments 'PenaltyBreakComment' and the penalty for breaking string
    +    literals 'PenaltyBreakString' also to 5.
    +
    +    Finally, we really care about not breaking the return type into its own
    +    line and we really care about not breaking before an open parenthesis.
    +    This avoids weird formatting like:
    +
    +       static const struct strbuf *
    +              a_really_really_large_function_name(struct strbuf resolved,
    +              const char *path, int flags)
    +
    +    or
    +
    +       static const struct strbuf *a_really_really_large_function_name(
    +                struct strbuf resolved, const char *path, int flags)
    +
    +    to instead have something more readable like:
    +
    +       static const struct strbuf *a_really_really_large_function_name(struct strbuf resolved,
    +              const char *path, int flags)
    +
    +    This is done by bumping the values of 'PenaltyReturnTypeOnItsOwnLine'
    +    and 'PenaltyBreakOpenParenthesis' to 300. This is so that we can allow a
    +    few characters above the 80 column limit to make code more readable.
     
         Signed-off-by: Karthik Nayak <[e-mail]>
     
      ## .clang-format ##
    -@@ .clang-format: 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
    +@@ .clang-format: KeepEmptyLinesAtTheStartOfBlocks: false
    + 
    + # Penalties
    + # This decides what order things should be done if a line is too long
    +-PenaltyBreakAssignment: 10
    +-PenaltyBreakBeforeFirstCallParameter: 30
    +-PenaltyBreakComment: 10
    ++PenaltyBreakAssignment: 5
    ++PenaltyBreakBeforeFirstCallParameter: 5
    ++PenaltyBreakComment: 5
    + PenaltyBreakFirstLessLess: 0
    +-PenaltyBreakString: 10
    +-PenaltyExcessCharacter: 100
    +-PenaltyReturnTypeOnItsOwnLine: 60
    ++PenaltyBreakOpenParenthesis: 300
    ++PenaltyBreakString: 5
    ++PenaltyExcessCharacter: 10
    ++PenaltyReturnTypeOnItsOwnLine: 300
      
    - # C Language specifics
    - Language: Cpp
    + # Don't sort #include's
    + SortIncludes: false
3:  6ebcd2690e = 2:  1586d53769 clang-format: align consecutive macro definitions
2:  b55d5d2c14 ! 3:  36a53299c1 clang-format: don't align expressions after linebreaks
    @@ Commit message
         Signed-off-by: Karthik Nayak <[e-mail]>
     
      ## .clang-format ##
    -@@ .clang-format: AlignConsecutiveDeclarations: false
    +@@ .clang-format: AlignConsecutiveMacros: true
      #   int cccccccc;
      AlignEscapedNewlines: Left