clang-format: Implement AlwaysBreakAfterDeclarationReturnType.

[zturner]

Summary

AlwaysBreakAfterDeclarationReturnType is like
AlwaysBreakAfterDefinitionReturnType, but inserts breaks
before declaration identifiers, not definition identifiers.

AlwaysBreakAfterDeclarationReturnType defaults to false
(old behaviour) for all styles.

[djasper]

include/clang/Format/Format.h

278–281

Hm.. I wonder whether we should really have two separate flags here or a single flag with an enum value, e.g. AlwaysBreakAfterReturnType with the values (No, DefinitionOnly, All).

Not sure we'll also need DeclarationOnly, but I don't think so.

lib/Format/ContinuationIndenter.cpp

129–130

I find the parentheses confusing.

lib/Format/TokenAnnotator.cpp

1546

Call this IsDefinition.

[strager]

include/clang/Format/Format.h

278–281

That would break all .clang-format files with AlwaysBreakAfterDefinitionReturnType specified, yes?

lib/Format/ContinuationIndenter.cpp

129–130

What's a better way to write this?

lib/Format/TokenAnnotator.cpp

1546

Will do.

[djasper]

include/clang/Format/Format.h

278–281

Well, the corresponding old strings in the configuration file can still set the right enum value. In that way, this should be fully backwards compatible.

lib/Format/ContinuationIndenter.cpp

129–130

I would just do:

if (Current.is(TT_FunctionDeclarationName) && State.Column < 6 &&
    !Style.AlwaysBreakAfterDefinitionReturnType &&
    !Style.AlwaysBreakAfterDeclarationReturnType)

[strager]

include/clang/Format/Format.h

278–281

So would there be two members in Style, or one? What happens when both are specified? Do any other flags behave this way?

lib/Format/ContinuationIndenter.cpp

129–130

Looking at this a bit more, we should probably do the same thing we do in TokenAnnotator::calculateFormattingInformation. How should I factor out the IsDefinition calculation?

[strager]

Fix test diff on Phabricator.

[strager]

Rebase. Refactor option to be a ReturnTypeBreakingStyle,
similar to AlwaysBreakAfterDefinitionReturnType.

[djasper]

docs/ClangFormatStyleOptions.rst

221 ↗

(On Diff #28753)

Do you think it'll ever make sense to break differently for declarations and definitions? I think having two entirely independent configuration flags gives us 9 combinations (assuming the three enum values will remain) out of which many will never be used.

I see two alternatives:
Add an additional bool flag "TreatDeclarationsLikeDefinitions" (name might not be ideal yet).
Change existing flag name to AlwaysBreakAfterReturnType and use five enum values (None, TopLevel, All, TopLevelDefinitions, AllDefinitions).

What do you think?

Sorry for being very picky on this. The problem is that these options stick around for a long time and we need to think about them carefully.

lib/Format/ContinuationIndenter.cpp

129–132

What do you mean exactly?

[strager]

(Sorry for the late feedback.)

docs/ClangFormatStyleOptions.rst

221 ↗

(On Diff #28753)

I think having two entirely independent configuration flags gives us 9 combinations (assuming the three enum values will remain) out of which many will never be used.

I agree that many combinations will be unused. I don't see why we should conflate the two if they are configured independently in practice, though.

Add an additional bool flag "TreatDeclarationsLikeDefinitions" (name might not be ideal yet).
Change existing flag name to AlwaysBreakAfterReturnType and use five enum values (None, TopLevel, All, TopLevelDefinitions, AllDefinitions).

What about styles where declarations have return types on their own line, but definitions don't? I don't see a reason to prevent that style.

lib/Format/ContinuationIndenter.cpp

129–132

See IsDefinition in lib/Format/TokenAnnotator.cpp introduced by this diff. The logic here and there should match, I think.

[strager]

Rebase.

[djasper]

docs/ClangFormatStyleOptions.rst

221–235 ↗

(On Diff #34947)

Same as I am arguing on some of your other patches. Fewer options are easier to maintain and easier to discover.

include/clang/Format/Format.h

133–136

Only one member in style, but parsing the configuration file should be backwards compatible. If both are present in the configuration file, the non-deprecated one should win. There is a few examples of where the configuration options are parsed for backwards compatibility, e.g. DerivePointerBinding.

lib/Format/ContinuationIndenter.cpp

129–132

You mean just the "!Line.Last->isOneOf(tok::semi, tok::comment);"? You can put this into a member function of AnnotatedLine maybe?

[strager]

docs/ClangFormatStyleOptions.rst

221–235 ↗

(On Diff #34947)

I think having separate options for separate cases is easier to maintain.

Current method (separate option):

FormatStyle::ReturnTypeBreakingStyle BreakStyle =
    Line.mightBeFunctionDefinition()
        ? Style.AlwaysBreakAfterDefinitionReturnType
        : Style.AlwaysBreakAfterDeclarationReturnType;
if ((BreakStyle == FormatStyle::DRTBS_All ||
     (BreakStyle == FormatStyle::DRTBS_TopLevel && Line.Level == 0)))
  Current->MustBreakBefore = true;

Proposed method:

auto BreakStyle = Style.AlwaysBreakAfterReturnType;
if (BreakStyle == FormatStyle::DRTBS_All ||
    (BreakStyle == FormatStyle::DRTBS_TopLevel && Line.Level == 0) ||
    (!Line->mightBeFunctionDefinition() &&
     (BreakStyle == FormatStyle::DRTBS_AllDeclarations) ||
     (BreakStyle == FormatStyle::DRTBS_TopLevelDeclarations &&
      Line.Level == 0)))
  Current->MustBreakBefore = true;

[strager]

Fix missing IsDefinition check in ContinuationIndenter::canBreak.

[djasper]

Maintenance is not about writing 7 lines of code correctly, but ensuring that all these options work correctly and in combination with all other options and that options remain discoverable and well documented. So, please change this to using the one enum with 5 configuration values.

[dawn]

Can someone please accept and commit this patch so we can use this feature? (After the objections are fixed of course). Thanks.

[djasper]

No. I really don't want to have these two completely independent flags, so the patch needs changes, before it can go in.

[zturner]

What needs to happen for this to go in? If I understand correctly, it is either:

1. Add a new option TreatDeclarationsLikeDefinitions
2. Merge this option into AlwaysBreakAfterDefinitionReturnType and make it an enum with 5 values.

In theory I think option 2 is the best, but are you concerned about backwards compatibility? It breaks anyone already using AlwaysBreakAfterDefinitionReturnType and forces them to update their style files. Is this an important consideration?

I don't really like option 1 because then we are tied to treating declarations like definitions for all current and future options including ones we haven't imagined yet. That might not be the best choice.

Let me know what to do, and I will finish out this patch if strager won't.

[djasper]

I think we can add the option AlwaysBreakAfterReturnType but still be backwards compatible. If clang-format finds only the old option in a configuration file, it can choose the appropriate value of the new option.

I also like using just this one option better than using the one option with the added bool option.

[zturner]

In D10370#309789, @djasper wrote:

I think we can add the option AlwaysBreakAfterReturnType but still be backwards compatible. If clang-format finds only the old option in a configuration file, it can choose the appropriate value of the new option.

I also like using just this one option better than using the one option with the added bool option.

What if it finds both the old option and the new option, but they conflict? Error out?

[djasper]

I'd just prefer the setting of the new option and ignore the old one then, but I don't think it matters to much. Warning or err'ing out also seems like a reasonable approach.

[zturner]

This patch was very old, so it didn't apply against ToT. So this initial update is just to rebase against ToT. I will make changes in a followup patch.

[zturner]

Attempt to address remaining issues in patch.

This is my first time touching anything having to do with clang, so there's a good chance I don't know what i'm doing and this needs more work. Let me know.

AlwaysBreakAfterDefinitionReturnType is used only as a way to initialize AlwaysBreakAfterReturnType, and is ignored during any actual logic.

[zturner]

ping

[djasper]

Generally looks good.

include/clang/Format/Format.h

166

Use tools/dump_format_style.py to update the documentation, too.

171

nit: top-level

lib/Format/TokenAnnotator.cpp

1642

No braces, I think.

lib/Format/TokenAnnotator.h

168

Some comment might help. E.g. at the very least, does that mean must break before or after "Token" (alternatively, name that Left or Right).

[zturner]

lib/Format/TokenAnnotator.h

168

Turns out this variable wasn't even used. I'll just delete it.

[Eugene.Zelenko]

Committed in rL256046.

include/clang/Format/Format.h

   /// single line.
   bool AllowShortLoopsOnASingleLine;
 
-  /// \brief Different ways to break after the function definition return type.
-  enum DefinitionReturnTypeBreakingStyle {
+  /// \brief Different ways to break after the function declaration or
+  /// definition return type.
+  enum ReturnTypeBreakingStyle {
     /// Break after return type automatically.
     /// \c PenaltyReturnTypeOnItsOwnLine is taken into account.
     DRTBS_None,

     DRTBS_TopLevel,
   };
 
+  /// \brief The function declaration return type breaking style to use.
+  ReturnTypeBreakingStyle AlwaysBreakAfterDeclarationReturnType;
+
   /// \brief The function definition return type breaking style to use.
-  DefinitionReturnTypeBreakingStyle AlwaysBreakAfterDefinitionReturnType;
+  ReturnTypeBreakingStyle AlwaysBreakAfterDefinitionReturnType;
 
   /// \brief If \c true, always break before multiline string literals.
   ///

            AllowShortIfStatementsOnASingleLine ==
                R.AllowShortIfStatementsOnASingleLine &&
            AllowShortLoopsOnASingleLine == R.AllowShortLoopsOnASingleLine &&
+           AlwaysBreakAfterDeclarationReturnType ==
+               R.AlwaysBreakAfterDeclarationReturnType &&
            AlwaysBreakAfterDefinitionReturnType ==
                R.AlwaysBreakAfterDefinitionReturnType &&
            AlwaysBreakBeforeMultilineStrings ==

lib/Format/ContinuationIndenter.cpp

 
   // Don't break after very short return types (e.g. "void") as that is often
   // unexpected.
-  if (Current.is(TT_FunctionDeclarationName) &&
-      Style.AlwaysBreakAfterDefinitionReturnType == FormatStyle::DRTBS_None &&
-      State.Column < 6)
-    return false;
+  if (Current.is(TT_FunctionDeclarationName) && State.Column < 6) {
+    FormatStyle::ReturnTypeBreakingStyle BreakStyle =
+        State.Line->mightBeFunctionDefinition()
+            ? Style.AlwaysBreakAfterDefinitionReturnType
+            : Style.AlwaysBreakAfterDeclarationReturnType;
+    if (BreakStyle == FormatStyle::DRTBS_None)
+      return false;
+  }
 
   return !State.Stack.back().NoLineBreak;
 }

lib/Format/Format.cpp

 };
 
 template <>
-struct ScalarEnumerationTraits<FormatStyle::DefinitionReturnTypeBreakingStyle> {
-  static void
-  enumeration(IO &IO, FormatStyle::DefinitionReturnTypeBreakingStyle &Value) {
+struct ScalarEnumerationTraits<FormatStyle::ReturnTypeBreakingStyle> {
+  static void enumeration(IO &IO, FormatStyle::ReturnTypeBreakingStyle &Value) {
     IO.enumCase(Value, "None", FormatStyle::DRTBS_None);
     IO.enumCase(Value, "All", FormatStyle::DRTBS_All);
     IO.enumCase(Value, "TopLevel", FormatStyle::DRTBS_TopLevel);

                    Style.AllowShortIfStatementsOnASingleLine);
     IO.mapOptional("AllowShortLoopsOnASingleLine",
                    Style.AllowShortLoopsOnASingleLine);
+    IO.mapOptional("AlwaysBreakAfterDeclarationReturnType",
+                   Style.AlwaysBreakAfterDeclarationReturnType);
     IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
                    Style.AlwaysBreakAfterDefinitionReturnType);
     IO.mapOptional("AlwaysBreakBeforeMultilineStrings",

   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AlwaysBreakTemplateDeclarations = false;

lib/Format/TokenAnnotator.h

     return startsWith(First, Tokens...);
   }
 
+  /// \c true if this line looks like a function definition instead of a
+  /// function declaration. Asserts MightBeFunctionDecl.
+  bool mightBeFunctionDefinition() const {
+    assert(MightBeFunctionDecl);
+    // FIXME: Line.Last points to other characters than tok::semi
+    // and tok::lbrace.
+    return !Last->isOneOf(tok::semi, tok::comment);
+  }
+
   FormatToken *First;
   FormatToken *Last;
 

lib/Format/TokenAnnotator.cpp

     Current->MustBreakBefore =
         Current->MustBreakBefore || mustBreakBefore(Line, *Current);
 
-    if ((Style.AlwaysBreakAfterDefinitionReturnType == FormatStyle::DRTBS_All ||
-         (Style.AlwaysBreakAfterDefinitionReturnType ==
-              FormatStyle::DRTBS_TopLevel &&
-          Line.Level == 0)) &&
-        InFunctionDecl && Current->is(TT_FunctionDeclarationName) &&
-        !Line.Last->isOneOf(tok::semi, tok::comment)) // Only for definitions.
-      // FIXME: Line.Last points to other characters than tok::semi
-      // and tok::lbrace.
+    if (!Current->MustBreakBefore && InFunctionDecl &&
+        Current->is(TT_FunctionDeclarationName)) {
+      FormatStyle::ReturnTypeBreakingStyle BreakStyle =
+          Line.mightBeFunctionDefinition()
+              ? Style.AlwaysBreakAfterDefinitionReturnType
+              : Style.AlwaysBreakAfterDeclarationReturnType;
+      if ((BreakStyle == FormatStyle::DRTBS_All ||
+           (BreakStyle == FormatStyle::DRTBS_TopLevel && Line.Level == 0)))
         Current->MustBreakBefore = true;
+    }
 
     Current->CanBreakBefore =
         Current->MustBreakBefore || canBreakBefore(Line, *Current);

unittests/Format/FormatTest.cpp

                "              \"c\";");
 }
 
+TEST_F(FormatTest, DeclarationReturnTypeBreakingStyle) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_TopLevel;
+  verifyFormat("class C {\n"
+               "  int f();\n"
+               "};\n"
+               "int\n"
+               "f();",
+               Style);
+  Style.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_All;
+  verifyFormat("class C {\n"
+               "  int\n"
+               "  f();\n"
+               "};\n"
+               "int\n"
+               "f();",
+               Style);
+  verifyFormat("const char *f(void) { return \"\"; }\n"
+               "const char *\n"
+               "bar(void);\n",
+               Style);
+  verifyFormat("template <class T> T *f(T &c) { return NULL; }\n"
+               "template <class T>\n"
+               "T *\n"
+               "f(T &c);\n",
+               Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Stroustrup;
+  verifyFormat("const char *f(void) { return \"\"; }\n"
+               "const char *\n"
+               "bar(void);\n",
+               Style);
+  verifyFormat("template <class T> T *f(T &c) { return NULL; }\n"
+               "template <class T>\n"
+               "T *\n"
+               "f(T &c);\n",
+               Style);
+}
+
 TEST_F(FormatTest, DefinitionReturnTypeBreakingStyle) {
   FormatStyle Style = getLLVMStyle();
   Style.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_TopLevel;

                Style);
 }
 
+TEST_F(FormatTest, AlwaysBreakAfterDeclarationAndDefinitionReturnTypeMixed) {
+  FormatStyle AfterType = getLLVMStyle();
+  AfterType.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_None;
+  AfterType.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
+  verifyFormat("void f(void) {\n" // No break here.
+               "  f();\n"
+               "  f();\n"
+               "}\n"
+               "void bar(void);\n", // No break here.
+               AfterType);
+  AfterType.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_All;
+  AfterType.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
+  verifyFormat("void f(void) {\n" // No break here.
+               "  f();\n"
+               "  f();\n"
+               "}\n"
+               "void\n"
+               "bar(void);\n", // Break here.
+               AfterType);
+  AfterType.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_None;
+  AfterType.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_All;
+  verifyFormat("void\n"
+               "f(void) {\n" // Break here.
+               "  f();\n"
+               "  f();\n"
+               "}\n"
+               "void bar(void);\n", // No break here.
+               AfterType);
+  AfterType.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_All;
+  AfterType.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_All;
+  verifyFormat("void\n"
+               "f(void) {\n" // Break here.
+               "  f();\n"
+               "  f();\n"
+               "}\n"
+               "void\n"
+               "bar(void);\n", // Break here.
+               AfterType);
+}
+
 TEST_F(FormatTest, AlwaysBreakBeforeMultilineStrings) {
   FormatStyle NoBreak = getLLVMStyle();
   NoBreak.AlwaysBreakBeforeMultilineStrings = false;

   CHECK_PARSE("BreakBeforeBraces: Custom", BreakBeforeBraces,
               FormatStyle::BS_Custom);
 
+  Style.AlwaysBreakAfterDeclarationReturnType = FormatStyle::DRTBS_All;
+  CHECK_PARSE("AlwaysBreakAfterDeclarationReturnType: None",
+              AlwaysBreakAfterDeclarationReturnType, FormatStyle::DRTBS_None);
+  CHECK_PARSE("AlwaysBreakAfterDeclarationReturnType: All",
+              AlwaysBreakAfterDeclarationReturnType, FormatStyle::DRTBS_All);
+  CHECK_PARSE("AlwaysBreakAfterDeclarationReturnType: TopLevel",
+              AlwaysBreakAfterDeclarationReturnType,
+              FormatStyle::DRTBS_TopLevel);
+
   Style.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_All;
   CHECK_PARSE("AlwaysBreakAfterDefinitionReturnType: None",
               AlwaysBreakAfterDefinitionReturnType, FormatStyle::DRTBS_None);