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

[Nathan Hjelm via Phabricator via cfe-commits]

hjelmn created this revision.
hjelmn added reviewers: krasimir, djasper.
hjelmn requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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) {}

disabled:

for (int i = 0; i < 10; ++i) {}

Signed-off-by: Nathan Hjelm 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D98429

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/FormatToken.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -12701,6 +12701,15 @@
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();
 
@@ -15610,6 +15619,7 @@
   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);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -,6 +,10 @@
 parseCSharpGenericTypeConstraint();
   }
   break;
+case tok::semi:
+  if (Contexts.back().ColonIsForRangeExpr)
+Tok->setType(TT_ForLoopSemiColon);
+  break;
 default:
   break;
 }
@@ -3345,6 +3349,8 @@
   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;
@@ -3995,6 +4001,8 @@
 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) ||
Index: clang/lib/Format/FormatToken.h
===
--- clang/lib/Format/FormatToken.h
+++ clang/lib/Format/FormatToken.h
@@ -46,6 +46,7 @@
   TYPE(DesignatedInitializerLSquare)   \
   TYPE(DesignatedInitializerPeriod)\
   TYPE(DictLiteral)\
+  TYPE(ForLoopSemiColon)   \
   TYPE(ForEachMacro)   \
   TYPE(FunctionAnnotationRParen)   \
   TYPE(FunctionDeclarationName)\
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -657,6 +657,8 @@
Style.SpaceBeforeCpp11BracedList);
 IO.mapOptional("SpaceBeforeCtorInitializerColon",
Style.SpaceBeforeCtorInitializerColon);
+IO.mapOptional("SpaceBeforeForLoopSemiColon",
+   Style.SpaceBeforeForLoopSemiColon);
 IO.mapOptional("SpaceBeforeInheritanceColon",
Style.SpaceBeforeInheritanceColon);
 IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
@@ -1026,6 +1028,7 @@
   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;
Index: clang/include/clang/Format/Format.h
=

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

[Nathan Hjelm via Phabricator via cfe-commits]

hjelmn updated this revision to Diff 330800.
hjelmn edited the summary of this revision.
hjelmn added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98429/new/

https://reviews.llvm.org/D98429

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/FormatToken.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -12701,6 +12701,17 @@
InvertedSpaceStyle);
 }
 
+TEST_F(FormatTest, ConfigurableSpaceBeforeForLoopSemiColon) {
+  FormatStyle NoSpaceStyle = getLLVMStyle();
+  verifyFormat("for (i = 0; i < 10; ++i) {\n}", NoSpaceStyle);
+  verifyFormat("for (int i = 0; auto a: b) {}", NoSpaceStyle);
+
+  FormatStyle Space = getLLVMStyle();
+  Space.SpaceBeforeForLoopSemiColon = true;
+  verifyFormat("for (i = 0 ; i < 10 ; ++i) {\n}", Space);
+  verifyFormat("for (int i = 0 ; auto a: b) {}", Space);
+}
+
 TEST_F(FormatTest, ConfigurableSpaceAroundPointerQualifiers) {
   FormatStyle Style = getLLVMStyle();
 
@@ -15610,6 +15621,7 @@
   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);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -,6 +,10 @@
 parseCSharpGenericTypeConstraint();
   }
   break;
+case tok::semi:
+  if (Contexts.back().ColonIsForRangeExpr)
+Tok->setType(TT_ForLoopSemiColon);
+  break;
 default:
   break;
 }
@@ -3345,6 +3349,8 @@
   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;
@@ -3995,6 +4001,8 @@
 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) ||
Index: clang/lib/Format/FormatToken.h
===
--- clang/lib/Format/FormatToken.h
+++ clang/lib/Format/FormatToken.h
@@ -46,6 +46,7 @@
   TYPE(DesignatedInitializerLSquare)   \
   TYPE(DesignatedInitializerPeriod)\
   TYPE(DictLiteral)\
+  TYPE(ForLoopSemiColon)   \
   TYPE(ForEachMacro)   \
   TYPE(FunctionAnnotationRParen)   \
   TYPE(FunctionDeclarationName)\
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -657,6 +657,8 @@
Style.SpaceBeforeCpp11BracedList);
 IO.mapOptional("SpaceBeforeCtorInitializerColon",
Style.SpaceBeforeCtorInitializerColon);
+IO.mapOptional("SpaceBeforeForLoopSemiColon",
+   Style.SpaceBeforeForLoopSemiColon);
 IO.mapOptional("SpaceBeforeInheritanceColon",
Style.SpaceBeforeInheritanceColon);
 IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
@@ -1026,6 +1028,7 @@
   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;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -2835,6 +2835,13 @@
   /// \endcode
   bool SpaceBeforeCtorIn

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

[Nathan Hjelm via Phabricator via cfe-commits]

hjelmn updated this revision to Diff 330834.
hjelmn added a comment.

Fixed a typo


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98429/new/

https://reviews.llvm.org/D98429

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/FormatToken.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -12701,6 +12701,17 @@
InvertedSpaceStyle);
 }
 
+TEST_F(FormatTest, ConfigurableSpaceBeforeForLoopSemiColon) {
+  FormatStyle NoSpaceStyle = getLLVMStyle();
+  verifyFormat("for (i = 0; i < 10; ++i) {\n}", NoSpaceStyle);
+  verifyFormat("for (int i = 0; auto a: b) {\n}", NoSpaceStyle);
+
+  FormatStyle Space = getLLVMStyle();
+  Space.SpaceBeforeForLoopSemiColon = true;
+  verifyFormat("for (i = 0 ; i < 10 ; ++i) {\n}", Space);
+  verifyFormat("for (int i = 0 ; auto a: b) {\n}", Space);
+}
+
 TEST_F(FormatTest, ConfigurableSpaceAroundPointerQualifiers) {
   FormatStyle Style = getLLVMStyle();
 
@@ -15610,6 +15621,7 @@
   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);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -,6 +,10 @@
 parseCSharpGenericTypeConstraint();
   }
   break;
+case tok::semi:
+  if (Contexts.back().ColonIsForRangeExpr)
+Tok->setType(TT_ForLoopSemiColon);
+  break;
 default:
   break;
 }
@@ -3345,6 +3349,8 @@
   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;
@@ -3995,6 +4001,8 @@
 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) ||
Index: clang/lib/Format/FormatToken.h
===
--- clang/lib/Format/FormatToken.h
+++ clang/lib/Format/FormatToken.h
@@ -46,6 +46,7 @@
   TYPE(DesignatedInitializerLSquare)   \
   TYPE(DesignatedInitializerPeriod)\
   TYPE(DictLiteral)\
+  TYPE(ForLoopSemiColon)   \
   TYPE(ForEachMacro)   \
   TYPE(FunctionAnnotationRParen)   \
   TYPE(FunctionDeclarationName)\
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -657,6 +657,8 @@
Style.SpaceBeforeCpp11BracedList);
 IO.mapOptional("SpaceBeforeCtorInitializerColon",
Style.SpaceBeforeCtorInitializerColon);
+IO.mapOptional("SpaceBeforeForLoopSemiColon",
+   Style.SpaceBeforeForLoopSemiColon);
 IO.mapOptional("SpaceBeforeInheritanceColon",
Style.SpaceBeforeInheritanceColon);
 IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
@@ -1026,6 +1028,7 @@
   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;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -2835,6 +2835,13 @@
   /// \endcode
   bool SpaceBeforeCtorInitializerColon;
 
+  /// If ``false``, spaces will be removed before semi-colons in for loops.
+  /// \cod

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

[Nathan Hjelm via Phabricator via cfe-commits]

hjelmn updated this revision to Diff 330865.
hjelmn added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98429/new/

https://reviews.llvm.org/D98429

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/FormatToken.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -12701,6 +12701,17 @@
InvertedSpaceStyle);
 }
 
+TEST_F(FormatTest, ConfigurableSpaceBeforeForLoopSemiColon) {
+  FormatStyle NoSpaceStyle = getLLVMStyle();
+  verifyFormat("for (i = 0; i < 10; ++i) {\n}", NoSpaceStyle);
+  verifyFormat("for (int i = 0; auto a : b) {\n}", NoSpaceStyle);
+
+  FormatStyle Space = getLLVMStyle();
+  Space.SpaceBeforeForLoopSemiColon = true;
+  verifyFormat("for (i = 0 ; i < 10 ; ++i) {\n}", Space);
+  verifyFormat("for (int i = 0 ; auto a : b) {\n}", Space);
+}
+
 TEST_F(FormatTest, ConfigurableSpaceAroundPointerQualifiers) {
   FormatStyle Style = getLLVMStyle();
 
@@ -15610,6 +15621,7 @@
   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);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -,6 +,10 @@
 parseCSharpGenericTypeConstraint();
   }
   break;
+case tok::semi:
+  if (Contexts.back().ColonIsForRangeExpr)
+Tok->setType(TT_ForLoopSemiColon);
+  break;
 default:
   break;
 }
@@ -3345,6 +3349,8 @@
   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;
@@ -3995,6 +4001,8 @@
 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) ||
Index: clang/lib/Format/FormatToken.h
===
--- clang/lib/Format/FormatToken.h
+++ clang/lib/Format/FormatToken.h
@@ -46,6 +46,7 @@
   TYPE(DesignatedInitializerLSquare)   \
   TYPE(DesignatedInitializerPeriod)\
   TYPE(DictLiteral)\
+  TYPE(ForLoopSemiColon)   \
   TYPE(ForEachMacro)   \
   TYPE(FunctionAnnotationRParen)   \
   TYPE(FunctionDeclarationName)\
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -657,6 +657,8 @@
Style.SpaceBeforeCpp11BracedList);
 IO.mapOptional("SpaceBeforeCtorInitializerColon",
Style.SpaceBeforeCtorInitializerColon);
+IO.mapOptional("SpaceBeforeForLoopSemiColon",
+   Style.SpaceBeforeForLoopSemiColon);
 IO.mapOptional("SpaceBeforeInheritanceColon",
Style.SpaceBeforeInheritanceColon);
 IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
@@ -1026,6 +1028,7 @@
   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;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -2835,6 +2835,13 @@
   /// \endcode
   bool SpaceBeforeCtorInitializerColon;
 
+  //

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

[Nathan Hjelm via Phabricator via cfe-commits]

hjelmn added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:168
 
+- Option ``SpaceBeforeForLoopSemiColon`` has been added to control whether
+  spaces will be added before the semi-colons in for loops.

HazardyKnusperkeks wrote:
> No need for change, but in the future I would prefer to add changes to the 
> end of the list. :)
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.



Comment at: clang/unittests/Format/FormatTest.cpp:12712
+  verifyFormat("for (i = 0 ; i < 10 ; ++i) {\n}", Space);
+  verifyFormat("for (int i = 0 ; auto a : b) {\n}", Space);
+}

HazardyKnusperkeks wrote:
> Okay that was unexpected for me, I thought the space would only apply to the 
> old `for`s.
> 
> 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.
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98429/new/

https://reviews.llvm.org/D98429

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan Hjelm via Phabricator via cfe-commits]

hjelmn added a comment.

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.




Comment at: clang/include/clang/Format/Format.h:2841
+  ///true:  false:
+  ///for (i = 0 ; i < 1 ; ++i) {}   vs. for (i = 0; i < 1; ++i) {}
+  /// \endcode

HazardyKnusperkeks wrote:
> Please also show the range based for here. Otherwise it may be surprising, it 
> would be for me.
Will do. Plan to get back to this this weekend.



Comment at: clang/lib/Format/TokenAnnotator.cpp:4004
 return false;
+  if (Right.is(TT_ForLoopSemiColon))
+return false;

curdeius wrote:
> 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. 
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.



Comment at: clang/unittests/Format/FormatTest.cpp:12712
+  verifyFormat("for (i = 0 ; i < 10 ; ++i) {\n}", Space);
+  verifyFormat("for (int i = 0 ; auto a : b) {\n}", Space);
+}

curdeius wrote:
> HazardyKnusperkeks wrote:
> > hjelmn wrote:
> > > HazardyKnusperkeks wrote:
> > > > Okay that was unexpected for me, I thought the space would only apply 
> > > > to the old `for`s.
> > > > 
> > > > 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.
> > > 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?
> > 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.
> 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.
Makes sense to me. Will do that and will close this and other comment once that 
is complete.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98429/new/

https://reviews.llvm.org/D98429

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits