[clang-format] Add new option to clang-format: SpaceBeforeForLoopSemiColon

[HazardyKnusperkeks]

Summary

The option SpaceBeforeForLoopSemiColon controls whether clang-format puts
a space before the semi-colons in a for loop. When disabled (default) there
is no change to the current behavior. When enabled clang-format will put a
space before semi-colons in for loops:

enabled:

for (int i = 0 ; i < 10 ; ++i) {}
for (int i = 0 ; auto a : b) {}

disabled:

for (int i = 0; i < 10; ++i) {}
for (int i = 0; auto a : b) {}

Signed-off-by: Nathan Hjelm <[e-mail]>

[HazardyKnusperkeks]

Please add a note to the release notes and also a test for a range based for with initializer (c++20).

[hjelmn]

Updated change log and added a test to cover range based for with initializer.

[hjelmn]

Fixed a typo

[hjelmn]

One more issue with the test. Made a mistake with whether there is a space before the colon.

[HazardyKnusperkeks]

clang/docs/ReleaseNotes.rst

168 ↗

(On Diff #330865)

No need for change, but in the future I would prefer to add changes to the end of the list. :)

clang/include/clang/Format/Format.h

2841

Please also show the range based for here. Otherwise it may be surprising, it would be for me.

clang/unittests/Format/FormatTest.cpp

12712

Okay that was unexpected for me, I thought the space would only apply to the old fors.

In that case please add while and maybe if with initializer. What should be discussed is if for and the other control statements with initializer should behave differently with regard to their initializer semicolon.

[hjelmn]

clang/docs/ReleaseNotes.rst

168 ↗

(On Diff #330865)

Opps. Should have asked the convention before putting it at the top. I will go ahead and move it to be consistent with LLVM practice.

clang/unittests/Format/FormatTest.cpp

12712

hmm, I think then I could rename this one to: SpaceBeforeCForLoopSemiColon which would then only add spaces in for(init;cond;increment)
then maybe SpaceAfterControlStatementInitializer or SpaceBeforeControlStatementSemiColon that controls the behavior for control statements with initializers.

How does that sound?

[HazardyKnusperkeks]

clang/docs/ReleaseNotes.rst

168 ↗

(On Diff #330865)

Don't know about LLVM practice, I'm just about 4 month here. Just my preference (and how I've done it until now).

clang/unittests/Format/FormatTest.cpp

12712

Apart from the names good. For the names I don't have anything really better right now.

But this is currently just my opinion, we should ask @MyDeveloperDay and @curdeius.

[curdeius]

Could you point me to a style guide that formats the code in this way please?

clang/include/clang/Format/Format.h

2838

A better description would be nice, here only the false case is described. I'd rather see something that describes the option is in general and adds that true will add/keep a space.

clang/lib/Format/TokenAnnotator.cpp

4004

Is this change really necessary? Is there a test for it?
I guess that a semicolon should return false because otherwise it could be put after the line break, but you certainly need a test for that.

clang/unittests/Format/FormatTest.cpp

12712

I would prefer to avoid multiplying the different options and regroup all of the control statements under a single one.
SpaceBeforeControlStatementSemicolon sounds acceptable with one nit, I'd use "Semicolon" (as a single word, with lowercase 'c') to match the usage in LLVM (e.g. in clang-tidy).

WDYT about a single option for all statements?
Another way is to add a separate option for each statement type. Or, use a (bitmask like) enum with a single option. The latter can be done in a backward compatible way in the future BTW.

[hjelmn]

This is something I have been doing for over 20 years now. Not sure when I initially picked it up but I find a space before the ;'s in a C for loop improves readability. It more clearly differentiates the different parts. I beleive the space before the colon in C++ range-based loops is based on the same readability improvement.

Anyway, I intend to use clang-format in a couple of C projects and want to make sure it doesn't try to remove the intentional formatting of the code. I have at least one more CL I need to open up for a bug I found in the formatting. Will try to get that one open as soon as I get this one finished.

clang/include/clang/Format/Format.h

2841

Will do. Plan to get back to this this weekend.

clang/lib/Format/TokenAnnotator.cpp

4004

Yes. The way I implemented it this I think it was necessary. I will have to double-check. I plan to re-do this with an enum so will try to avoid extraneous changes.

clang/unittests/Format/FormatTest.cpp

12712

Makes sense to me. Will do that and will close this and other comment once that is complete.

[MyDeveloperDay]

Can you show for (;;) {} in your tests?

[HazardyKnusperkeks]

More than a year silence.

clang/docs/ClangFormatStyleOptions.rst

**SpaceBeforeCtorInitializerColon** (``bool``)
   If ``false``, spaces will be removed before constructor initializer
   colon.
 
   .. code-block:: c++
 
      true:                                  false:
      Foo::Foo() : a(a) {}                   Foo::Foo(): a(a) {}
 
+**SpaceBeforeForLoopSemiColon** (``bool``)
+  If ``false``, spaces will be removed before for loop semi-colon.
+
+  .. code-block:: c++
+
+     true:                                  false:
+     for (i = 0 ; i < 10 ; ++i) {}  vs.     for (i = 0; i < 10; ++i) {}
+
 **SpaceBeforeInheritanceColon** (``bool``)
   If ``false``, spaces will be removed before inheritance colon.
 
   .. code-block:: c++
 
      true:                                  false:
      class Foo : Bar {}             vs.     class Foo: Bar {}
 

clang/include/clang/Format/Format.h

   /// If ``false``, spaces will be removed before constructor initializer
   /// colon.
   /// \code
   ///    true:                                  false:
   ///    Foo::Foo() : a(a) {}                   Foo::Foo(): a(a) {}
   /// \endcode
   bool SpaceBeforeCtorInitializerColon;
 
+  /// If ``false``, spaces will be removed before semi-colons in for loops.
+  /// \code
+  ///    true:                                  false:
+  ///    for (i = 0 ; i < 1 ; ++i) {}   vs.     for (i = 0; i < 1; ++i) {}
+  /// \endcode
+  bool SpaceBeforeForLoopSemiColon;
+
   /// If ``false``, spaces will be removed before inheritance colon.
   /// \code
   ///    true:                                  false:
   ///    class Foo : Bar {}             vs.     class Foo: Bar {}
   /// \endcode
   bool SpaceBeforeInheritanceColon;
 
   /// Different ways to put a space before opening parentheses.
return AccessModifierOffset == R.AccessModifierOffset &&
            SpaceAfterCStyleCast == R.SpaceAfterCStyleCast &&
            SpaceAfterLogicalNot == R.SpaceAfterLogicalNot &&
            SpaceAfterTemplateKeyword == R.SpaceAfterTemplateKeyword &&
            SpaceBeforeAssignmentOperators == R.SpaceBeforeAssignmentOperators &&
            SpaceBeforeCaseColon == R.SpaceBeforeCaseColon &&
            SpaceBeforeCpp11BracedList == R.SpaceBeforeCpp11BracedList &&
            SpaceBeforeCtorInitializerColon ==
                R.SpaceBeforeCtorInitializerColon &&
+           SpaceBeforeForLoopSemiColon == R.SpaceBeforeForLoopSemiColon &&
            SpaceBeforeInheritanceColon == R.SpaceBeforeInheritanceColon &&
            SpaceBeforeParens == R.SpaceBeforeParens &&
            SpaceAroundPointerQualifiers == R.SpaceAroundPointerQualifiers &&
            SpaceBeforeRangeBasedForLoopColon ==
                R.SpaceBeforeRangeBasedForLoopColon &&
            SpaceInEmptyBlock == R.SpaceInEmptyBlock &&
            SpaceInEmptyParentheses == R.SpaceInEmptyParentheses &&
            SpacesBeforeTrailingComments == R.SpacesBeforeTrailingComments &&

clang/lib/Format/Format.cpp

IO.mapOptional("SpaceAfterTemplateKeyword",
                    Style.SpaceAfterTemplateKeyword);
     IO.mapOptional("SpaceBeforeAssignmentOperators",
                    Style.SpaceBeforeAssignmentOperators);
     IO.mapOptional("SpaceBeforeCaseColon", Style.SpaceBeforeCaseColon);
     IO.mapOptional("SpaceBeforeCpp11BracedList",
                    Style.SpaceBeforeCpp11BracedList);
     IO.mapOptional("SpaceBeforeCtorInitializerColon",
                    Style.SpaceBeforeCtorInitializerColon);
+    IO.mapOptional("SpaceBeforeForLoopSemiColon",
+                   Style.SpaceBeforeForLoopSemiColon);
     IO.mapOptional("SpaceBeforeInheritanceColon",
                    Style.SpaceBeforeInheritanceColon);
     IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
     IO.mapOptional("SpaceAroundPointerQualifiers",
                    Style.SpaceAroundPointerQualifiers);
     IO.mapOptional("SpaceBeforeRangeBasedForLoopColon",
                    Style.SpaceBeforeRangeBasedForLoopColon);
     IO.mapOptional("SpaceInEmptyBlock", Style.SpaceInEmptyBlock);
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.SpacesInCStyleCastParentheses = false;
   LLVMStyle.SpacesInLineCommentPrefix = {/*Minimum=*/1, /*Maximum=*/-1u};
   LLVMStyle.SpaceAfterCStyleCast = false;
   LLVMStyle.SpaceAfterLogicalNot = false;
   LLVMStyle.SpaceAfterTemplateKeyword = true;
   LLVMStyle.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Default;
   LLVMStyle.SpaceBeforeCaseColon = false;
   LLVMStyle.SpaceBeforeCtorInitializerColon = true;
+  LLVMStyle.SpaceBeforeForLoopSemiColon = false;
   LLVMStyle.SpaceBeforeInheritanceColon = true;
   LLVMStyle.SpaceBeforeParens = FormatStyle::SBPO_ControlStatements;
   LLVMStyle.SpaceBeforeRangeBasedForLoopColon = true;
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
   LLVMStyle.SpaceBeforeSquareBrackets = false;
   LLVMStyle.BitFieldColonSpacing = FormatStyle::BFCS_Both;
   LLVMStyle.SpacesInAngles = false;

clang/lib/Format/FormatToken.h

#define LIST_TOKEN_TYPES                                                       \
   TYPE(ConflictEnd)                                                            \
   TYPE(ConflictStart)                                                          \
   TYPE(ConstraintJunctions)                                                    \
   TYPE(CtorInitializerColon)                                                   \
   TYPE(CtorInitializerComma)                                                   \
   TYPE(DesignatedInitializerLSquare)                                           \
   TYPE(DesignatedInitializerPeriod)                                            \
   TYPE(DictLiteral)                                                            \
+  TYPE(ForLoopSemiColon)                                                       \
   TYPE(ForEachMacro)                                                           \
   TYPE(FunctionAnnotationRParen)                                               \
   TYPE(FunctionDeclarationName)                                                \
   TYPE(FunctionLBrace)                                                         \
   TYPE(FunctionTypeLParen)                                                     \
   TYPE(ImplicitStringLiteral)                                                  \
   TYPE(InheritanceColon)                                                       \
   TYPE(InheritanceComma)                                                       \

clang/lib/Format/TokenAnnotator.cpp

case tok::identifier:
         parseHasInclude();
       }
       if (Style.isCSharp() && Tok->is(Keywords.kw_where) && Tok->Next &&
           Tok->Next->isNot(tok::l_paren)) {
         Tok->setType(TT_CSharpGenericTypeConstraint);
         parseCSharpGenericTypeConstraint();
       }
       break;
+    case tok::semi:
+      if (Contexts.back().ColonIsForRangeExpr)
+        Tok->setType(TT_ForLoopSemiColon);
+      break;
     default:
       break;
     }
     return true;
   }
 
   void parseCSharpGenericTypeConstraint() {
     int OpenAngleBracketsCount = 0;
if (Right.is(TT_ObjCBlockLParen))
     return true;
   if (Right.is(TT_CtorInitializerColon))
     return Style.SpaceBeforeCtorInitializerColon;
   if (Right.is(TT_InheritanceColon) && !Style.SpaceBeforeInheritanceColon)
     return false;
   if (Right.is(TT_RangeBasedForLoopColon) &&
       !Style.SpaceBeforeRangeBasedForLoopColon)
     return false;
+  if (Right.is(TT_ForLoopSemiColon) && Style.SpaceBeforeForLoopSemiColon)
+    return true;
   if (Left.is(TT_BitFieldColon))
     return Style.BitFieldColonSpacing == FormatStyle::BFCS_Both ||
            Style.BitFieldColonSpacing == FormatStyle::BFCS_After;
   if (Right.is(tok::colon)) {
     if (Line.First->isOneOf(tok::kw_default, tok::kw_case))
       return Style.SpaceBeforeCaseColon;
     if (!Right.getNextNonComment() || Right.getNextNonComment()->is(tok::semi))
       return false;
if (Left.ClosesTemplateDeclaration || Left.is(TT_FunctionAnnotationRParen))
     return true;
   if (Right.isOneOf(TT_RangeBasedForLoopColon, TT_OverloadedOperatorLParen,
                     TT_OverloadedOperator))
     return false;
   if (Left.is(TT_RangeBasedForLoopColon))
     return true;
   if (Right.is(TT_RangeBasedForLoopColon))
     return false;
+  if (Right.is(TT_ForLoopSemiColon))
+    return false;
   if (Left.is(TT_TemplateCloser) && Right.is(TT_TemplateOpener))
     return true;
   if (Left.isOneOf(TT_TemplateCloser, TT_UnaryOperator) ||
       Left.is(tok::kw_operator))
     return false;
   if (Left.is(tok::equal) && !Right.isOneOf(tok::kw_default, tok::kw_delete) &&
       Line.Type == LT_VirtualFunctionDecl && Left.NestingLevel == 0)
     return false;

clang/unittests/Format/FormatTest.cpp

verifyFormat("switch (allBraces) {\n"
                "}\n"
                "default : {\n"
                "  break;\n"
                "}\n"
                "}",
                InvertedSpaceStyle);
 }
 
+TEST_F(FormatTest, ConfigurableSpaceBeforeForLoopSemiColon) {
+  FormatStyle NoSpaceStyle = getLLVMStyle();
+  verifyFormat("for (i = 0; i < 10; ++i) {\n}", NoSpaceStyle);
+
+  FormatStyle Space = getLLVMStyle();
+  Space.SpaceBeforeForLoopSemiColon = true;
+  verifyFormat("for (i = 0 ; i < 10 ; ++i) {\n}", Space);
+}
+
 TEST_F(FormatTest, ConfigurableSpaceAroundPointerQualifiers) {
   FormatStyle Style = getLLVMStyle();
 
   Style.PointerAlignment = FormatStyle::PAS_Left;
   Style.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Default;
   verifyFormat("void* const* x = NULL;", Style);
 
 #define verifyQualifierSpaces(Code, Pointers, Qualifiers)                      \
TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(SpacesInCStyleCastParentheses);
   CHECK_PARSE_BOOL(SpaceAfterCStyleCast);
   CHECK_PARSE_BOOL(SpaceAfterTemplateKeyword);
   CHECK_PARSE_BOOL(SpaceAfterLogicalNot);
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
   CHECK_PARSE_BOOL(SpaceBeforeCaseColon);
   CHECK_PARSE_BOOL(SpaceBeforeCpp11BracedList);
   CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
+  CHECK_PARSE_BOOL(SpaceBeforeForLoopSemiColon);
   CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon);
   CHECK_PARSE_BOOL(SpaceBeforeRangeBasedForLoopColon);
   CHECK_PARSE_BOOL(SpaceBeforeSquareBrackets);
   CHECK_PARSE_BOOL(UseCRLF);
 
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterCaseLabel);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterClass);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterEnum);