[clang-format] Add a new extra command line option for ide-specific formatting

[yvvan]

Summary

In order to use clang-format for formatting/indenting code in IDE we need to add some tricks which are never needed for clang-format itself.

Let's start with the option which keeps all existing line breaks when the flag is set. This option is available with extra parameter for reformat() function.

The next steps could be for example - adding dummy text to empty lines to indent them or applying format only for the code before the cursor (for incomplete code). There are much more tricks which could also be considered later.

[krasimir]

In general there's a high bar for adding new style options to clang-format:
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options

[yvvan]

Do you know the better way to accomplish my aim than adding an option to libFormat? For example making a dependent library which serves a little different purpose than libFormat itself or something simpler?

[krasimir]

In D53072#1268883, @yvvan wrote:

Do you know the better way to accomplish my aim than adding an option to libFormat? For example making a dependent library which serves a little different purpose than libFormat itself or something simpler?

I don't know about the specific use-case, but in general a fail-safe way to force a line break in C++ code is to add an empty line comment on the preceding line.

[djasper]

I don't quite understand. What you are describing should already be the behavior with ColumnLimit=0 and I think your test should pass without the new option. Doesn't it?

[yvvan]

In D53072#1342363, @djasper wrote:

I don't quite understand. What you are describing should already be the behavior with ColumnLimit=0 and I think your test should pass without the new option. Doesn't it?

As far as I see how it works column limit does not prevent the shrinking behavior.

In D53072#1342353, @krasimir wrote:

In D53072#1268883, @yvvan wrote:

Do you know the better way to accomplish my aim than adding an option to libFormat? For example making a dependent library which serves a little different purpose than libFormat itself or something simpler?

I don't know about the specific use-case, but in general a fail-safe way to force a line break in C++ code is to add an empty line comment on the preceding line.

This is close but will not work for the case when more than two lines are packed together. So it requires to put the empty comment on the each line ending which is not too nice (if i'm not missing something)

[djasper]

$ cat /tmp/test.cc
int foo(int a,
           int b) {
      f();
  }

$ clang-format -style="{ColumnLimit: 0}" /tmp/test.cc
int foo(int a,
        int b) {
  f();
}

Is this not what you want? If so, in what way?

[yvvan]

In D53072#1348033, @djasper wrote:

$ cat /tmp/test.cc
int foo(int a,
           int b) {
      f();
  }

$ clang-format -style="{ColumnLimit: 0}" /tmp/test.cc
int foo(int a,
        int b) {
  f();
}

Is this not what you want? If so, in what way?

Ok, this might mean that my test is bad and does not test what I want (there are more cases which are not formatted the way I want with ColumnLimit: 0). But since the patch is never to be submitted I don't see a reason to improve it.

[djasper]

Without understanding this in more detail I do not know how to move forward with this. What this patch is describing is what should already be the case with ColumnLimit set to zero. If it isn't this might be a bug or there might be a different way to move forward. However, the flag as is does not make sense to me without more information.

[yvvan]

@djasper
Ok, if there's a possible way to go forward I will find time to provide a better test which behaves differently depending on this check. Also my current patch is a bit bigger than this version.

[yvvan]

The tests are improved - now they actually act differently with and without the introduced flag. Also few more cases are covered (see the second added test).

[djasper]

Ok, but this behavior is still intended. You are setting clang-format to a format where it is breaking after binary operators and then added a break before a binary operator. clang-format assumes that this is not intended and that you will want this cleaned up.

E.g.:

$ cat /tmp/format.cc
if ( a
      && b) {
}

$ clang-format -style="{ColumnLimit: 0, BreakBeforeBinaryOperators: All}" /tmp/format.cc
if (a
    && b) {
}

$ clang-format -style="{ColumnLimit: 0}" /tmp/format.cc
if (a && b) {
}

I believe that this behavior is right and we should not work around it with an additional flag.

[yvvan]

New approach - title and summary are updated.

[sammccall]

You may be interested in D60605, which is a related idea (adding incremental format-and-indent-on-type to clangd).

These are opposite extremes in some sense: this patch integrates deeply into clang-format and that patch entirely layers on top of it, by transforming the source code (in hacky ways, in some cases).

I think the cleanest approach is probably some combination: in particular a "preserve line break at point" primitive does seem useful, and even if source code transforms are used, putting a nice API on this in clang-format would be nice. (I'm not sure a separate command-line-tool is justified, though).

If you haven't run into these yet, here are some fun issues I ran into:

• clang-format bails out when brackets aren't sufficiently matched, incremental formatting needs to work in such cases
• incremental formatting sometimes wants to make edits both immediately before and after the cursor. tooling::Replacements can't represent this, they will be merged and destroy precise cursor position information.
• user expectations when breaking between () are fairly clear. {} is the same when it acts as a list, but not when it acts as a block!

[yvvan]

@sammccall

Having a separate tool is nice because it allows the client to make it plugable. clang-format sometimes changes options quite significantly and it can be nice if you have a choice which version to pick, otherwise it might be unable to read the configuration you have.

[ilya-biryukov]

In D53072#1478575, @yvvan wrote:

@sammccall

Having a separate tool is nice because it allows the client to make it plugable. clang-format sometimes changes options quite significantly and it can be nice if you have a choice which version to pick, otherwise it might be unable to read the configuration you have.

I second Sam's concerns about introducing a new tool.
Could you list the use-cases that clang-format-ide would cover that clang-format doesn't?
How is introducing a new tool different from adding a new formatting option to clang-format?

[yvvan]

@ilya-biryukov
I don't really care how it's used from the tool side. I'm also fine to have a new option in clang-format itself. That's why this review is here - to ask for opinions. It's easy to remove that "ide" part from this patch and just add an option for clang-format instead.
Do you have any other remarks/concerns apart from that?

[sammccall]

My feedback would be:

• I definitely think more control over preserving line breaks would be useful. Actually preserving *blank* lines is an important property. If you see D60605, there are a lot of cases where clang-format wants to delete the line you just added. Maybe we can make MaxEmptyLinesToKeep: 9999999 work in more cases.
• I'm not convinced you need/want to preserve *all* line breaks, isn't it just one? (where the user pressed enter)
• It's unclear why "extraformattingoptions" is needed, rather than just the usual formatting options, or some other parameter. It would be nice to avoid a new library entry point (overload) if possible.
• as far as command-line interface goes, this definitely feels more like a flag than a new tool

[yvvan]

In D53072#1496317, @sammccall wrote:

My feedback would be:

• I definitely think more control over preserving line breaks would be useful. Actually preserving *blank* lines is an important property. If you see D60605, there are a lot of cases where clang-format wants to delete the line you just added. Maybe we can make MaxEmptyLinesToKeep: 9999999 work in more cases.
• I'm not convinced you need/want to preserve *all* line breaks, isn't it just one? (where the user pressed enter)
• It's unclear why "extraformattingoptions" is needed, rather than just the usual formatting options, or some other parameter. It would be nice to avoid a new library entry point (overload) if possible.
• as far as command-line interface goes, this definitely feels more like a flag than a new tool

Thanks for the feedback!
'to preserve *all* line breaks, isn't it just one' - it's just much easier code wise, you don't need to take the current position into account. I can look how I can make it for only one line.
'unclear why "extraformattingoptions" is needed' - the idea behind is that we can add more ide-specific options later without introducing new reformat parameters.
I will definitely remove a separate tool, it seems nobody sees the need of such thing.

[yvvan]

@sammccall
I can't avoid adding extra formatting options because my first attempt to introduce an ordinary clang-format option faced resistance because of not fitting the clang-format purpose to format files.

[yvvan]

Sorry for unrelated formatting changes - that's clang-format's work :)

I've removed the extra executable.

I don't know how to force that behavior only for the given line (for that I need someone who can help) but in our usecase we should not care if we apply the rule to the whole file or only to the current line since we are only interested in one single Replacement which we can anyways filter afterwards.

[yvvan]

Some misleading reformatting fixed.

include/clang/Format/Format.h

   ///        VeryLongImportsAreAnnoying,
   ///    } from 'some/module.js'
   ///
   ///    false:
   ///    import {VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying, VeryLongImportsAreAnnoying,} from "some/module.js"
   /// \endcode
   bool JavaScriptWrapImports;
 
+  /// If true, no line breaks are optimized out (works only with ColumnLimit = 0)
+  /// \code
+  ///    true:                                  false:
+  ///    if (a                          vs.     if (a && b) {
+  ///        && b) {
+  ///      bar();                                 bar();
+  ///    }                                      }
+  /// \endcode
+  bool KeepLineBreaksForNonEmptyLines;
+
   /// If true, the empty line at the start of blocks is kept.
   /// \code
   ///    true:                                  false:
   ///    if (foo) {                     vs.     if (foo) {
   ///                                             bar();
   ///      bar();                               }
   ///    }
   /// \endcode
return AccessModifierOffset == R.AccessModifierOffset &&
            IncludeStyle.IncludeBlocks == R.IncludeStyle.IncludeBlocks &&
            IncludeStyle.IncludeCategories == R.IncludeStyle.IncludeCategories &&
            IndentCaseLabels == R.IndentCaseLabels &&
            IndentPPDirectives == R.IndentPPDirectives &&
            IndentWidth == R.IndentWidth && Language == R.Language &&
            IndentWrappedFunctionNames == R.IndentWrappedFunctionNames &&
            JavaScriptQuotes == R.JavaScriptQuotes &&
            JavaScriptWrapImports == R.JavaScriptWrapImports &&
+           KeepLineBreaksForNonEmptyLines == R.KeepLineBreaksForNonEmptyLines &&
            KeepEmptyLinesAtTheStartOfBlocks ==
                R.KeepEmptyLinesAtTheStartOfBlocks &&
            MacroBlockBegin == R.MacroBlockBegin &&
            MacroBlockEnd == R.MacroBlockEnd &&
            MaxEmptyLinesToKeep == R.MaxEmptyLinesToKeep &&
            NamespaceIndentation == R.NamespaceIndentation &&
            ObjCBinPackProtocolList == R.ObjCBinPackProtocolList &&
            ObjCBlockIndentWidth == R.ObjCBlockIndentWidth &&

lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex);
     IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
     IO.mapOptional("IndentPPDirectives", Style.IndentPPDirectives);
     IO.mapOptional("IndentWidth", Style.IndentWidth);
     IO.mapOptional("IndentWrappedFunctionNames",
                    Style.IndentWrappedFunctionNames);
     IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
     IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
+    IO.mapOptional("KeepLineBreaksForNonEmptyLines",
+                   Style.KeepLineBreaksForNonEmptyLines);
     IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks",
                    Style.KeepEmptyLinesAtTheStartOfBlocks);
     IO.mapOptional("MacroBlockBegin", Style.MacroBlockBegin);
     IO.mapOptional("MacroBlockEnd", Style.MacroBlockEnd);
     IO.mapOptional("MaxEmptyLinesToKeep", Style.MaxEmptyLinesToKeep);
     IO.mapOptional("NamespaceIndentation", Style.NamespaceIndentation);
     IO.mapOptional("ObjCBinPackProtocolList", Style.ObjCBinPackProtocolList);
     IO.mapOptional("ObjCBlockIndentWidth", Style.ObjCBlockIndentWidth);
FormatStyle getLLVMStyle() {
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
   LLVMStyle.IndentWrappedFunctionNames = false;
   LLVMStyle.IndentWidth = 2;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
   LLVMStyle.JavaScriptWrapImports = true;
   LLVMStyle.TabWidth = 8;
   LLVMStyle.MaxEmptyLinesToKeep = 1;
+  LLVMStyle.KeepLineBreaksForNonEmptyLines = false;
   LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true;
   LLVMStyle.NamespaceIndentation = FormatStyle::NI_None;
   LLVMStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Auto;
   LLVMStyle.ObjCBlockIndentWidth = 2;
   LLVMStyle.ObjCSpaceAfterProperty = false;
   LLVMStyle.ObjCSpaceBeforeProtocolList = true;
   LLVMStyle.PointerAlignment = FormatStyle::PAS_Right;
   LLVMStyle.SpacesBeforeTrailingComments = 1;
FormatStyle getGoogleStyle(FormatStyle::LanguageKind Language) {
   GoogleStyle.AlwaysBreakBeforeMultilineStrings = true;
   GoogleStyle.AlwaysBreakTemplateDeclarations = FormatStyle::BTDS_Yes;
   GoogleStyle.ConstructorInitializerAllOnOneLineOrOnePerLine = true;
   GoogleStyle.DerivePointerAlignment = true;
   GoogleStyle.IncludeStyle.IncludeCategories = {
       {"^<ext/.*\\.h>", 2}, {"^<.*\\.h>", 1}, {"^<.*", 2}, {".*", 3}};
   GoogleStyle.IncludeStyle.IncludeIsMainRegex = "([-_](test|unittest))?$";
   GoogleStyle.IndentCaseLabels = true;
+  GoogleStyle.KeepLineBreaksForNonEmptyLines = false;
   GoogleStyle.KeepEmptyLinesAtTheStartOfBlocks = false;
   GoogleStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Never;
   GoogleStyle.ObjCSpaceAfterProperty = false;
   GoogleStyle.ObjCSpaceBeforeProtocolList = true;
   GoogleStyle.PointerAlignment = FormatStyle::PAS_Left;
   GoogleStyle.RawStringFormats = {
       {
           FormatStyle::LK_Cpp,

lib/Format/UnwrappedLineFormatter.cpp

 #endif
 
 /// Base class for classes that format one \c AnnotatedLine.
 class LineFormatter {
 public:
   LineFormatter(ContinuationIndenter *Indenter, WhitespaceManager *Whitespaces,
                 const FormatStyle &Style,
                 UnwrappedLineFormatter *BlockFormatter)
-      : Indenter(Indenter), Whitespaces(Whitespaces), Style(Style),
+      : Indenter(Indenter), Style(Style), Whitespaces(Whitespaces),
         BlockFormatter(BlockFormatter) {}
   virtual ~LineFormatter() {}
 
   /// Formats an \c AnnotatedLine and returns the penalty.
   ///
   /// If \p DryRun is \c false, directly applies the changes.
   virtual unsigned formatLine(const AnnotatedLine &Line,
                               unsigned FirstIndent,
bool formatChildren(LineState &State, bool NewLine, bool DryRun,
     const FormatToken *LBrace = State.NextToken->getPreviousNonComment();
     FormatToken &Previous = *State.NextToken->Previous;
     if (!LBrace || LBrace->isNot(tok::l_brace) ||
         LBrace->BlockKind != BK_Block || Previous.Children.size() == 0)
       // The previous token does not open a block. Nothing to do. We don't
       // assert so that we can simply call this function for all tokens.
       return true;
 
-    if (NewLine) {
+    if (NewLine || (Previous.Children[0]->First->MustBreakBefore &&
+                    Style.KeepLineBreaksForNonEmptyLines)) {
       int AdditionalIndent = State.Stack.back().Indent -
                              Previous.Children[0]->Level * Style.IndentWidth;
 
       Penalty +=
           BlockFormatter->format(Previous.Children, DryRun, AdditionalIndent,
                                  /*FixBadIndentation=*/true);
       return true;
     }
bool formatChildren(LineState &State, bool NewLine, bool DryRun,
     Penalty +=
         formatLine(*Child, State.Column + 1, /*FirstStartColumn=*/0, DryRun);
 
     State.Column += 1 + Child->Last->TotalLength;
     return true;
   }
 
   ContinuationIndenter *Indenter;
+  const FormatStyle &Style;
 
 private:
   WhitespaceManager *Whitespaces;
-  const FormatStyle &Style;
   UnwrappedLineFormatter *BlockFormatter;
 };
 
 /// Formatter that keeps the existing line breaks.
 class NoColumnLimitLineFormatter : public LineFormatter {
 public:
   NoColumnLimitLineFormatter(ContinuationIndenter *Indenter,
                              WhitespaceManager *Whitespaces,
                              const FormatStyle &Style,
                              UnwrappedLineFormatter *BlockFormatter)
       : LineFormatter(Indenter, Whitespaces, Style, BlockFormatter) {}
 
   /// Formats the line, simply keeping all of the input's line breaking
   /// decisions.
   unsigned formatLine(const AnnotatedLine &Line, unsigned FirstIndent,
                       unsigned FirstStartColumn, bool DryRun) override {
     assert(!DryRun);
     LineState State = Indenter->getInitialState(FirstIndent, FirstStartColumn,
                                                 &Line, /*DryRun=*/false);
     while (State.NextToken) {
       bool Newline =
           Indenter->mustBreak(State) ||
-          (Indenter->canBreak(State) && State.NextToken->NewlinesBefore > 0);
+          (State.NextToken->NewlinesBefore > 0 && Indenter->canBreak(State));
       unsigned Penalty = 0;
       formatChildren(State, Newline, /*DryRun=*/false, Penalty);
       Indenter->addTokenToState(State, Newline, /*DryRun=*/false);
     }
     return 0;
   }
 };
 

lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::nextToken(int LevelDifference) {
   flushComments(isOnNewLine(*FormatTok));
   pushToken(FormatTok);
   FormatToken *Previous = FormatTok;
   if (Style.Language != FormatStyle::LK_JavaScript)
     readToken(LevelDifference);
   else
     readTokenWithJavaScriptASI();
   FormatTok->Previous = Previous;
+  if (FormatTok->NewlinesBefore && Style.KeepLineBreaksForNonEmptyLines)
+    FormatTok->MustBreakBefore = true;
 }
 
 void UnwrappedLineParser::distributeComments(
     const SmallVectorImpl<FormatToken *> &Comments,
     const FormatToken *NextTok) {
   // Whether or not a line comment token continues a line is controlled by
   // the method continuesLineCommentSection, with the following caveat:
   //

unittests/Format/FormatTest.cpp

EXPECT_EQ("class Foo\n"
                    "{\n"
                    "  Foo()\n"
                    "  {\n"
                    "  }\n"
                    "\n"
                    "  void funk() {}\n"
                    "};",
                    Style));
+
+  Style.KeepLineBreaksForNonEmptyLines = true;
+  Style.ColumnLimit = 0;
+  EXPECT_EQ("if (a\n"
+            "    && b) {\n"
+            "}",
+            format("if (a\n"
+                   "    && b) {\n"
+                   "}",
+                   Style));
+
+  EXPECT_EQ("[]() {\n"
+            "  foo(); }",
+            format("[]() {\n"
+                   "foo(); }",
+                   Style));
 }
 
 TEST_F(FormatTest, RecognizesBinaryOperatorKeywords) {
   verifyFormat("x = (a) and (b);");
   verifyFormat("x = (a) or (b);");
   verifyFormat("x = (a) bitand (b);");
   verifyFormat("x = (a) bitor (b);");
   verifyFormat("x = (a) not_eq (b);");