Page MenuHomePhabricator

[clang-format] Add ability to wrap braces after multi-line control statements
ClosedPublic

Authored by mitchell-stellar on Oct 1 2019, 2:00 PM.

Details

Summary

Change the BraceWrappingFlags' AfterControlStatement from a bool to an enum with three values:

  • "Never": This is the default, and does not do any brace wrapping after control statements.
  • "MultiLine": This only wraps braces after multi-line control statements (this really only happens when a ColumnLimit is specified).
  • "Always": This always wraps braces after control statements.

The first and last options are backwards-compatible with "false" and "true", respectively.

The new "MultiLine" option is useful for when a wrapped control statement's indentation matches the subsequent block's indentation. It makes it easier to see at a glance where the control statement ends and where the block's code begins. For example:

if (
  foo
  && bar )
{
  baz();
}

vs.

if (
  foo
  && bar ) {
  baz();
}

Short control statements (1 line) do not wrap the brace to the next line, e.g.

if (foo) {
  bar();
} else {
  baz();
}

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2019, 2:00 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MyDeveloperDay requested changes to this revision.Oct 2 2019, 1:25 AM
MyDeveloperDay added a subscriber: MyDeveloperDay.

Firstly thank you for the patch..

Prior authors have pointed out that new styles need to be supported by a large public style guide as evidence as to why clang-format should support such changes. (if you can find one that points to this being a preference I would reference it here for completeness, see note below)

My personal view is we are getting to the point where so many large projects are already clang-formated to some extent, the definition of the style guide could be based on what clang-format can do and not on necessarily makes the code more readable. What I have begun to notice is some projects on github where clang-format can't support their style simply don't have a .clang-format file. (and that could often be because of 1 or 2 issues which prevent them from adopting it fully.) what is also strange is that that may have a .clang-tidy file which points to them potentially being keen to adopt the clang tools, but why not clang-format when its clearly the simplest to adaopt first)

The reason I ask is to uncover the motivation for adding the change? is it something you need or a nice to have? (adding that to the revision descriptrion helps the reviewers understand the motivation)

We'd normally ask that people be willing to support their revision should there be any need to fix issues, I'd also say that clang-format desperately needs additional developers to help fix issues and in particular to do code reviews,
I only say this because this is seemingly your first revision and whilst I think this revision LGTM I'd hope we could call on you to help maintain clang-format moving forward (given you have an ability to understand how it works in order to do this patch)

Now regarding this revision. It wasn't initially clear what this change gave us, I now I look deeper I see it as being the ability to break the brace when the condition (if,for,catch,etc..) gets itself broken over multiple lines, leading to the view that:

if (aaaaaaaaaaaaaaaaaaaaa 
    && bbbbbbbbbbbbbbb 
    && cccccccccccccc)
{
    return true;
}
else {
    return false;
}

is easier to read than:

if (aaaaaaaaaaaaaaaaaaaaa 
    && bbbbbbbbbbbbbbb 
    && cccccccccccccc) {
    return true;
}
else {
    return false;
}

whilst still recognizing the desire for the compact version when the condition is small.

if (true) {
    return true;
}
else {
    return false;
}

To be honest, I'd be more than happy to see this style as it most closely matches what I used to do all the time before I switched to clang-formatting on save all the time (which meant I couldn't), That was to use a different bracing style based on the complexity of the condition.

I guess my question is do you thin any of the other bracing wrapping rules need the same level of control? (I can't think of any off the top of my head)

Apart from the missing documentation this LGTM (which is why I requested changes), I'm not sure if others have a different view (maybe give them some time to review this), but you'll find it can be quite quiet here. (which is why we need more volunteers to do reviews).

If you can find that style guide evidence!

clang/include/clang/Format/Format.h
817 ↗(On Diff #222695)

You are missing a doc change in the clang/docs/ClangStyleOptions.rst file (which I believe you can autogenerate from the Format.h), this needs to be part of the patch

clang/lib/Format/Format.cpp
643 ↗(On Diff #222695)

Nit: this a drive by comment, but I feel this is really hard to read.. and not something that needs to be done in this revision but this feels like it needs to be formatted differently

{
/*AfterClass=*/false,
/*AfterEnum=*/false
/*AfterControlStatement=*/FormatStyle::BWACS_Never,
.....
}

because without going back and checking that the AfterControl statement is the 3rd bool I simply can't see what is on and what is off.

This revision now requires changes to proceed.Oct 2 2019, 1:25 AM
MyDeveloperDay added a project: Restricted Project.Oct 2 2019, 1:25 AM
MyDeveloperDay added a reviewer: klimek.
mitchell-stellar edited the summary of this revision. (Show Details)

Thanks for the review. I have added documentation updates.

I do not have a public style guide to reference. My company just switched to auto-clang-formatting all of our code, and this patch addresses one of our biggest pain points: readability. When the opening brace of a multi-line control statement is not wrapped, we simply cannot tell at first glance where the control statement ends and where the body begins due to our indentation settings.

I think the fact that you yourself see the benefit of this change speaks to its viability.

I cannot think of any other brace wrapping rules that need this level of control. Our code base consists of millions of lines of code, so I'm pretty sure we would have found another instance.

I would be willing to support this patch and further revisions if this gets into master, as our company will depend on this feature.

klimek added a comment.Oct 2 2019, 8:08 AM

Thanks for the review. I have added documentation updates.

I do not have a public style guide to reference. My company just switched to auto-clang-formatting all of our code, and this patch addresses one of our biggest pain points: readability. When the opening brace of a multi-line control statement is not wrapped, we simply cannot tell at first glance where the control statement ends and where the body begins due to our indentation settings.

I think the fact that you yourself see the benefit of this change speaks to its viability.

I cannot think of any other brace wrapping rules that need this level of control. Our code base consists of millions of lines of code, so I'm pretty sure we would have found another instance.

I would be willing to support this patch and further revisions if this gets into master, as our company will depend on this feature.

I personally don't think the feature is worth it (readability is something we really like to assert on without good measurements), but I will defer to MyDeveloperDay.

As I mentioned, its worth getting evidence from a large public project...

https://wiki.blender.org/index.php/Dev:Doc/Code_Style

Blender public repo on github.com has 90 Forks, 299 * and 90,000 commits

From their style guide

Exceptions to Braces on Same Line
When flow control (if, for, while) is broken up into multiple lines, it reads better to have the brace on its own lines.

/* Don't: */
    if ((very_long_function_check(that, has, many, arguments)) &&
        (another_very_long_function(some, more, arguments)) &&
        some_more_checks_this_gets_tedious(but, its, readable)) {
        some_other_check_this_could_be_confusing ? func_a() : func_b();
    }

/* Do: */
    if ((very_long_function_check(that, has, many, arguments)) &&
        (another_very_long_function(some, more, arguments)) &&
        some_more_checks_this_gets_tedious(but, its, readable))
    {
        (some_other_check_this_could_be_confusing && n) ? func_a() : func_b();
    }

https://developer.blender.org/T53211

# NOTE: At time of writing (10/30/2017) not all formatting can be made exactly
# like current Blender sources, so a few compromises are made:
#
#   1. Newline after : in switch statements: clang-format will put the { on
#      the same line. This is due to a limitation in clang-format; it does not
#      support adding the newline after cases in switch statements.
#   2. Nested preprocessor directives don't get proper indentation after the
#      '#'. See IndentPPDirectives, which is supported in clang-format from
#      LLVM6, but not LLVM5. It is included below but commented out.
#   3. Special case of { position on if statements where the condition covers
#      more than one line. clang-format is an all or nothing formatter in this
#      case; so unfortunately the case of
#
#      if (long_condition_here() ||
#          long_condition_here() ||
#          long_condition_here() ||
#          long_condition_here())
#      {
#          ...
#      }
#
#      will become
#
#      if (long_condition_here() ||
#          long_condition_here() ||
#          long_condition_here() ||
#          long_condition_here()) {
#          ...
#      }
#

@klimek thanks for offering for me to make the decision. I think given the fact that blender has it in their style guide and that they are clearly working around clang-formats inability to handle this case, I'm minded to LGTM this revision

I hope you are ok with that. given that @mitchell-stellar is prepared to support it, and I'm happy enough that I could fix something with that I hope you don't mind.

@mitchell-stellar are you happy with the name OnlyMultiLine? the reason I ask is:

AlwaysBreakBeforeMultilineStrings uses just MultiLine

BTDS_MultiLine (in configuration: MultiLine) Force break after template declaration only when the following declaration spans multiple lines.

I think given this won't be a standard true/false parameter users having to remember the "Only" part seems unnecessary

MyDeveloperDay retitled this revision from clang-format: Add ability to wrap braces after multi-line control statements to [clang-format] Add ability to wrap braces after multi-line control statements.Oct 2 2019, 12:04 PM

@MyDeveloperDay I am not wedded to "OnlyMultiLine". I picked it, as it seemed reasonable to me, but if you think another string expresses the intent better, then feel free to use it.

MyDeveloperDay requested changes to this revision.Oct 2 2019, 1:04 PM

Looks good but I think the option would be better as just MultiLine, remember to mark the comments "Done" when you are complete

clang/docs/ClangFormatStyleOptions.rst
798 ↗(On Diff #222835)

Change to MultiLine to match AlwaysBreakBeforeMultilineStrings

This revision now requires changes to proceed.Oct 2 2019, 1:04 PM

Renamed "OnlyMultiLine" option to "MultiLine", per reviewer request.

mitchell-stellar edited the summary of this revision. (Show Details)Oct 2 2019, 1:57 PM
mitchell-stellar marked 4 inline comments as done.Oct 2 2019, 2:00 PM
mitchell-stellar added inline comments.
clang/lib/Format/Format.cpp
643 ↗(On Diff #222695)

I'm going to leave this as-is, because before, I was still forced to look-up what each true or false value was assigned to.

mitchell-stellar marked an inline comment as done.Oct 2 2019, 2:01 PM
MyDeveloperDay accepted this revision.Oct 2 2019, 2:20 PM

LGTM and Thank you.

clang/lib/Format/Format.cpp
643 ↗(On Diff #222695)

that's fine, it's always the problem with comments they can also get out of date, to be honest, if every element was an enum then it would be:

{
FormatStyle::BWAC::Never,
FormatStyle::BWAE::Never,
.....

and when the structure changes the compiler would tell us if we had it wrong, but thats for another time, your enum will help begin that process.

This revision is now accepted and ready to land.Oct 2 2019, 2:20 PM

Rebased against master, and also added test case for 'do' statements.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2019, 11:41 AM
MyDeveloperDay added inline comments.Oct 25 2019, 8:13 AM
cfe/trunk/docs/ClangFormatStyleOptions.rst
812

This text cannot be generated by dump_format_style.py in clang/docs/tools from the Format.h

I think this is the first time we've seen an enum being used in a structure, as such dump_format_style doesn't support pulling the text in automatically.

The next person who runs dump_format_style could easily end up blowing away your documentation.