[PATCH] D68346: [clang-format] Add new option to add spaces around conditions

[Mitchell via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG26748a321e20: [clang-format] Add new option to add spaces 
around conditions Summary: This… (authored by mitchell-stellar).

Changed prior to commit:
  https://reviews.llvm.org/D68346?vs=225345=231930#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68346

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  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
@@ -12555,6 +12555,7 @@
   CHECK_PARSE_BOOL(SpacesInParentheses);
   CHECK_PARSE_BOOL(SpacesInSquareBrackets);
   CHECK_PARSE_BOOL(SpacesInAngles);
+  CHECK_PARSE_BOOL(SpacesInConditionalStatement);
   CHECK_PARSE_BOOL(SpaceInEmptyBlock);
   CHECK_PARSE_BOOL(SpaceInEmptyParentheses);
   CHECK_PARSE_BOOL(SpacesInContainerLiterals);
@@ -14880,6 +14881,22 @@
   verifyFormat("auto lambda = [ = a]() { a = 2; };", AlignStyle);
 }
 
+ TEST_F(FormatTest, SpacesInConditionalStatement) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.SpacesInConditionalStatement = true;
+  verifyFormat("for ( int i = 0; i; i++ )\n  continue;", Spaces);
+  verifyFormat("if ( !a )\n  return;", Spaces);
+  verifyFormat("if ( a )\n  return;", Spaces);
+  verifyFormat("if constexpr ( a )\n  return;", Spaces);
+  verifyFormat("switch ( a )\ncase 1:\n  return;", Spaces);
+  verifyFormat("while ( a )\n  return;", Spaces);
+  verifyFormat("while ( (a && b) )\n  return;", Spaces);
+  verifyFormat("do {\n} while ( 1 != 0 );", Spaces);
+  // Check that space on the left of "::" is inserted as expected at beginning
+  // of condition.
+  verifyFormat("while ( ::func() )\n  return;", Spaces);
+}
+
 TEST_F(FormatTest, AlternativeOperators) {
   // Test case for ensuring alternate operators are not
   // combined with their right most neighbour.
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2592,6 +2592,13 @@
   Right.ParameterCount > 0);
 }
 
+/// Returns \c true if the token is followed by a boolean condition, \c false
+/// otherwise.
+static bool isKeywordWithCondition(const FormatToken ) {
+  return Tok.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
+ tok::kw_constexpr);
+};
+
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine ,
   const FormatToken ,
   const FormatToken ) {
@@ -2610,6 +2617,15 @@
   (Left.is(tok::l_brace) && Left.BlockKind != BK_Block &&
Right.is(tok::r_brace) && Right.BlockKind != BK_Block))
 return Style.SpaceInEmptyParentheses;
+  if (Style.SpacesInConditionalStatement) {
+if (Left.is(tok::l_paren) && Left.Previous &&
+isKeywordWithCondition(*Left.Previous))
+  return true;
+if (Right.is(tok::r_paren) && Right.MatchingParen &&
+Right.MatchingParen->Previous &&
+isKeywordWithCondition(*Right.MatchingParen->Previous))
+  return true;
+  }
   if (Left.is(tok::l_paren) || Right.is(tok::r_paren))
 return (Right.is(TT_CastRParen) ||
 (Left.MatchingParen && Left.MatchingParen->is(TT_CastRParen)))
@@ -3044,7 +3060,8 @@
 // The identifier might actually be a macro name such as ALWAYS_INLINE. If
 // this turns out to be too lenient, add analysis of the identifier itself.
 return Right.WhitespaceRange.getBegin() != Right.WhitespaceRange.getEnd();
-  if (Right.is(tok::coloncolon) && !Left.isOneOf(tok::l_brace, tok::comment))
+  if (Right.is(tok::coloncolon) &&
+  !Left.isOneOf(tok::l_brace, tok::comment, tok::l_paren))
 return (Left.is(TT_TemplateOpener) &&
 Style.Standard < FormatStyle::LS_Cpp11) ||
!(Left.isOneOf(tok::l_paren, tok::r_paren, tok::l_square,
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -537,6 +537,8 @@
 IO.mapOptional("SpacesBeforeTrailingComments",
Style.SpacesBeforeTrailingComments);
 IO.mapOptional("SpacesInAngles", Style.SpacesInAngles);
+IO.mapOptional("SpacesInConditionalStatement",
+   Style.SpacesInConditionalStatement);
 IO.mapOptional("SpacesInContainerLiterals",
Style.SpacesInContainerLiterals);
 IO.mapOptional("SpacesInCStyleCastParentheses",
@@ -817,6 +819,7 @@
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
   LLVMStyle.SpaceBeforeSquareBrackets = false;
   LLVMStyle.SpacesInAngles = false;
+  

[PATCH] D68346: [clang-format] Add new option to add spaces around conditions

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:14386
+  verifyFormat("while ( a )\n  return;", Spaces);
+  verifyFormat("while ( (a && b) )\n  return;", Spaces);
+  verifyFormat("do {\n} while ( 1 != 0 );", Spaces);

is this what you expect? or should it be `while ( ( a && b ) )`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68346



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

[PATCH] D68346: [clang-format] Add new option to add spaces around conditions

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Can you rebase it and add a release note in docs/ReleaseNote.rst


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68346



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

[PATCH] D68346: [clang-format] Add new option to add spaces around conditions

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:14382
+  verifyFormat("if ( !a )\n  return;", Spaces);
+  verifyFormat("if ( a )\n  return;", Spaces);
+  verifyFormat("if constexpr ( a )\n  return;", Spaces);

show "else if" example


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68346



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

[PATCH] D68346: [clang-format] Add new option to add spaces around conditions

[Tim Wojtulewicz via Phabricator via cfe-commits]

timwoj added a comment.

Thanks for the consideration on committer access, but I'm going to have to pass 
for the time being.

That said, can someone land this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68346



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

[PATCH] D68346: [clang-format] Add new option to add spaces around conditions

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Thank you for the patch.

If you plan to continue to submit patches I think it would be worthwhile if you 
applied for commit access, This is not the first revision from you and we need 
more people who are willing to help with clang-format to fix bug and do code 
reviews of other peoples work.

You clearly have an interested and an understanding of how clang-format works 
and a desire to help make it EVEN better. It is also good to get access in 
order to maintain the work you do,

As the cut over to github I believe is tomorrow at the LLVM meeting it might be 
worth waiting until after that point to work out what the procedure to obtain 
access will be as I'm unclear if getting permission will be the same as 
outlined here

https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68346



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

[PATCH] D68346: [clang-format] Add new option to add spaces around conditions

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68346



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

[PATCH] D68346: [clang-format] Add new option to add spaces around conditions

[Mitchell via Phabricator via cfe-commits]

mitchell-stellar accepted this revision.
mitchell-stellar added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68346



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

[PATCH] D68346: [clang-format] Add new option to add spaces around conditions

[Tim Wojtulewicz via Phabricator via cfe-commits]

timwoj updated this revision to Diff 225345.
timwoj added a comment.

Fix ordering


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68346

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  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
@@ -12165,6 +12165,7 @@
   CHECK_PARSE_BOOL(SpacesInParentheses);
   CHECK_PARSE_BOOL(SpacesInSquareBrackets);
   CHECK_PARSE_BOOL(SpacesInAngles);
+  CHECK_PARSE_BOOL(SpacesInConditionalStatement);
   CHECK_PARSE_BOOL(SpaceInEmptyBlock);
   CHECK_PARSE_BOOL(SpaceInEmptyParentheses);
   CHECK_PARSE_BOOL(SpacesInContainerLiterals);
@@ -14373,6 +14374,23 @@
   verifyFormat("auto lambda = [ = a]() { a = 2; };", AlignStyle);
 }
 
+TEST_F(FormatTest, SpacesInConditionalStatement) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.SpacesInConditionalStatement = true;
+  verifyFormat("for ( int i = 0; i; i++ )\n  continue;", Spaces);
+  verifyFormat("if ( !a )\n  return;", Spaces);
+  verifyFormat("if ( a )\n  return;", Spaces);
+  verifyFormat("if constexpr ( a )\n  return;", Spaces);
+  verifyFormat("switch ( a )\ncase 1:\n  return;", Spaces);
+  verifyFormat("while ( a )\n  return;", Spaces);
+  verifyFormat("while ( (a && b) )\n  return;", Spaces);
+  verifyFormat("do {\n} while ( 1 != 0 );", Spaces);
+
+  // Check that space on the left of "::" is inserted as expected at beginning
+  // of condition.
+  verifyFormat("while ( ::func() )\n  return;", Spaces);
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2489,6 +2489,13 @@
   Right.ParameterCount > 0);
 }
 
+/// Returns \c true if the token is followed by a boolean condition, \c false
+/// otherwise.
+static bool isKeywordWithCondition(const FormatToken ) {
+  return Tok.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
+ tok::kw_constexpr);
+};
+
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine ,
   const FormatToken ,
   const FormatToken ) {
@@ -2505,6 +2512,16 @@
 return Right.is(tok::hash);
   if (Left.is(tok::l_paren) && Right.is(tok::r_paren))
 return Style.SpaceInEmptyParentheses;
+  if (Style.SpacesInConditionalStatement) {
+if (Left.is(tok::l_paren) && Left.Previous &&
+isKeywordWithCondition(*Left.Previous) )
+  return true;
+if (Right.is(tok::r_paren) &&
+Right.MatchingParen &&
+Right.MatchingParen->Previous &&
+isKeywordWithCondition(*Right.MatchingParen->Previous))
+  return true;
+  }
   if (Left.is(tok::l_paren) || Right.is(tok::r_paren))
 return (Right.is(TT_CastRParen) ||
 (Left.MatchingParen && Left.MatchingParen->is(TT_CastRParen)))
@@ -2903,7 +2920,8 @@
 // The identifier might actually be a macro name such as ALWAYS_INLINE. If
 // this turns out to be too lenient, add analysis of the identifier itself.
 return Right.WhitespaceRange.getBegin() != Right.WhitespaceRange.getEnd();
-  if (Right.is(tok::coloncolon) && !Left.isOneOf(tok::l_brace, tok::comment))
+  if (Right.is(tok::coloncolon) &&
+  !Left.isOneOf(tok::l_brace, tok::comment, tok::l_paren))
 return (Left.is(TT_TemplateOpener) &&
 Style.Standard < FormatStyle::LS_Cpp11) ||
!(Left.isOneOf(tok::l_paren, tok::r_paren, tok::l_square,
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -511,6 +511,7 @@
 IO.mapOptional("SpacesBeforeTrailingComments",
Style.SpacesBeforeTrailingComments);
 IO.mapOptional("SpacesInAngles", Style.SpacesInAngles);
+IO.mapOptional("SpacesInConditionalStatement", Style.SpacesInConditionalStatement);
 IO.mapOptional("SpacesInContainerLiterals",
Style.SpacesInContainerLiterals);
 IO.mapOptional("SpacesInCStyleCastParentheses",
@@ -775,6 +776,7 @@
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
   LLVMStyle.SpacesInAngles = false;
+  LLVMStyle.SpacesInConditionalStatement = false;
 
   LLVMStyle.PenaltyBreakAssignment = prec::Assignment;
   LLVMStyle.PenaltyBreakComment = 300;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ 

[PATCH] D68346: [clang-format] Add new option to add spaces around conditions

[Mitchell via Phabricator via cfe-commits]

mitchell-stellar requested changes to this revision.
mitchell-stellar added inline comments.
This revision now requires changes to proceed.



Comment at: clang/include/clang/Format/Format.h:1955
+  /// \endcode
+  bool SpacesInConditionalStatement;
+

Nitpick: this option is still not in alphabetical order. Please re-order it. 
Otherwise, this looks good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68346



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

[PATCH] D68346: [clang-format] Add new option to add spaces around conditions

[Tim Wojtulewicz via Phabricator via cfe-commits]

timwoj added a comment.

In D68346#1708384 , @timwoj wrote:

> Changed option name to SpacesInConditionalStatement, added 
> isKeywordWithCondition method, added some extra tests


Sorry for the delay on getting back to this. I'm not the original author and I 
was waiting for changes from him, and then I slacked on getting them submitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68346



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

[PATCH] D68346: [clang-format] Add new option to add spaces around conditions

[Tim Wojtulewicz via Phabricator via cfe-commits]

timwoj updated this revision to Diff 224890.
timwoj added a comment.

Changed option name to SpacesInConditionalStatement, added 
isKeywordWithCondition method, added some extra tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68346

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  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
@@ -12165,6 +12165,7 @@
   CHECK_PARSE_BOOL(SpacesInParentheses);
   CHECK_PARSE_BOOL(SpacesInSquareBrackets);
   CHECK_PARSE_BOOL(SpacesInAngles);
+  CHECK_PARSE_BOOL(SpacesInConditionalStatement);
   CHECK_PARSE_BOOL(SpaceInEmptyBlock);
   CHECK_PARSE_BOOL(SpaceInEmptyParentheses);
   CHECK_PARSE_BOOL(SpacesInContainerLiterals);
@@ -14373,6 +14374,23 @@
   verifyFormat("auto lambda = [ = a]() { a = 2; };", AlignStyle);
 }
 
+TEST_F(FormatTest, SpacesInConditionalStatement) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.SpacesInConditionalStatement = true;
+  verifyFormat("for ( int i = 0; i; i++ )\n  continue;", Spaces);
+  verifyFormat("if ( !a )\n  return;", Spaces);
+  verifyFormat("if ( a )\n  return;", Spaces);
+  verifyFormat("if constexpr ( a )\n  return;", Spaces);
+  verifyFormat("switch ( a )\ncase 1:\n  return;", Spaces);
+  verifyFormat("while ( a )\n  return;", Spaces);
+  verifyFormat("while ( (a && b) )\n  return;", Spaces);
+  verifyFormat("do {\n} while ( 1 != 0 );", Spaces);
+
+  // Check that space on the left of "::" is inserted as expected at beginning
+  // of condition.
+  verifyFormat("while ( ::func() )\n  return;", Spaces);
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2489,6 +2489,13 @@
   Right.ParameterCount > 0);
 }
 
+/// Returns \c true if the token is followed by a boolean condition, \c false
+/// otherwise.
+static bool isKeywordWithCondition(const FormatToken ) {
+  return Tok.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
+ tok::kw_constexpr);
+};
+
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine ,
   const FormatToken ,
   const FormatToken ) {
@@ -2505,6 +2512,16 @@
 return Right.is(tok::hash);
   if (Left.is(tok::l_paren) && Right.is(tok::r_paren))
 return Style.SpaceInEmptyParentheses;
+  if (Style.SpacesInConditionalStatement) {
+if (Left.is(tok::l_paren) && Left.Previous &&
+isKeywordWithCondition(*Left.Previous) )
+  return true;
+if (Right.is(tok::r_paren) &&
+Right.MatchingParen &&
+Right.MatchingParen->Previous &&
+isKeywordWithCondition(*Right.MatchingParen->Previous))
+  return true;
+  }
   if (Left.is(tok::l_paren) || Right.is(tok::r_paren))
 return (Right.is(TT_CastRParen) ||
 (Left.MatchingParen && Left.MatchingParen->is(TT_CastRParen)))
@@ -2903,7 +2920,8 @@
 // The identifier might actually be a macro name such as ALWAYS_INLINE. If
 // this turns out to be too lenient, add analysis of the identifier itself.
 return Right.WhitespaceRange.getBegin() != Right.WhitespaceRange.getEnd();
-  if (Right.is(tok::coloncolon) && !Left.isOneOf(tok::l_brace, tok::comment))
+  if (Right.is(tok::coloncolon) &&
+  !Left.isOneOf(tok::l_brace, tok::comment, tok::l_paren))
 return (Left.is(TT_TemplateOpener) &&
 Style.Standard < FormatStyle::LS_Cpp11) ||
!(Left.isOneOf(tok::l_paren, tok::r_paren, tok::l_square,
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -511,6 +511,7 @@
 IO.mapOptional("SpacesBeforeTrailingComments",
Style.SpacesBeforeTrailingComments);
 IO.mapOptional("SpacesInAngles", Style.SpacesInAngles);
+IO.mapOptional("SpacesInConditionalStatement", Style.SpacesInConditionalStatement);
 IO.mapOptional("SpacesInContainerLiterals",
Style.SpacesInContainerLiterals);
 IO.mapOptional("SpacesInCStyleCastParentheses",
@@ -775,6 +776,7 @@
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
   LLVMStyle.SpacesInAngles = false;
+  LLVMStyle.SpacesInConditionalStatement = false;
 
   LLVMStyle.PenaltyBreakAssignment = prec::Assignment;
   LLVMStyle.PenaltyBreakComment = 300;
Index: clang/include/clang/Format/Format.h

[PATCH] D68346: [clang-format] Add new option to add spaces around conditions

[Mitchell via Phabricator via cfe-commits]

mitchell-stellar requested changes to this revision.
mitchell-stellar added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:2290
 
+**SpacesAroundConditions** (``bool``)
+  If ``true``, spaces will be inserted around if/for/while (and similar) 
conditions.

`SpacesInConditionalStatement` is probably a better name, remaining consistent 
with existing `SpacesIn*` options, as well as indicating it only affects the 
entire conditional statement, and not individual conditions within the 
statement.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2511
+  return t->isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
+tok::kw_switch, tok::kw_constexpr, TT_ForEachMacro);
+};

It seems that you are mixing statement tokens `if`, `while`, etc. with 
preprocessor and macro tokens. Going by the documentation, this is unexpected.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2919
+  if (Right.is(tok::coloncolon) &&
+  !Left.isOneOf(tok::l_brace, tok::comment, tok::l_paren))
 return (Left.is(TT_TemplateOpener) &&

MyDeveloperDay wrote:
> I'm not sure I understand this change so you added a `tok::l_paren` here? but 
> its not just for your style, so something else must have changed. Did you run 
> all FormatTests?
> 
> one of the tests you add need to test why you added this here
This is not covered by your tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68346



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

[PATCH] D68346: [clang-format] Add new option to add spaces around conditions

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay requested changes to this revision.
MyDeveloperDay added inline comments.
This revision now requires changes to proceed.



Comment at: clang/include/clang/Format/Format.h:1950
+  /// similar) conditions.
+  bool SpacesAroundConditions;
+

its position in the file and the documentation is not quite alphabetic, given 
you are close i'd make it so



Comment at: clang/include/clang/Format/Format.h:2088
SpacesInSquareBrackets == R.SpacesInSquareBrackets &&
+   SpacesAroundConditions == R.SpacesAroundConditions &&
Standard == R.Standard && TabWidth == R.TabWidth &&

move upto just before SpacesBeforeTrailingComments 



Comment at: clang/lib/Format/Format.cpp:520
 IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
+IO.mapOptional("SpacesAroundConditions", Style.SpacesAroundConditions);
 IO.mapOptional("Standard", Style.Standard);

Same here



Comment at: clang/lib/Format/Format.cpp:779
   LLVMStyle.SpacesInAngles = false;
+  LLVMStyle.SpacesAroundConditions = false;
 

Sorry move to line 771-772



Comment at: clang/lib/Format/TokenAnnotator.cpp:2509
+  if (Style.SpacesAroundConditions) {
+const auto is_cond_kw = [&](const FormatToken *t) {
+  return t->isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,

this isn't really the normal naming convention for variables, I think it would 
be `IsCondKw`  I think its clearer without using abbreviations,

why not just make this a function? does the Lambda really give us anything here 
in such a big function other than clutter?

`static bool isConditionKeyword()`



Comment at: clang/lib/Format/TokenAnnotator.cpp:2919
+  if (Right.is(tok::coloncolon) &&
+  !Left.isOneOf(tok::l_brace, tok::comment, tok::l_paren))
 return (Left.is(TT_TemplateOpener) &&

I'm not sure I understand this change so you added a `tok::l_paren` here? but 
its not just for your style, so something else must have changed. Did you run 
all FormatTests?

one of the tests you add need to test why you added this here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68346



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

[PATCH] D68346: Clang-format: Add new option to add spaces around conditions

[Tim Wojtulewicz via Phabricator via cfe-commits]

timwoj created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This diff adds a new option SpacesAroundConditions that inserts spaces inside 
the braces for conditional statements. It's used by the Zeek project 
(https://github.com/zeek/zeek) as described in their style guide 
(https://github.com/zeek/zeek-docs/blob/master/devel/style_guide.rst#conditional-expressions)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68346

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  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
@@ -12177,6 +12177,7 @@
   CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
   CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon);
   CHECK_PARSE_BOOL(SpaceBeforeRangeBasedForLoopColon);
+  CHECK_PARSE_BOOL(SpacesAroundConditions);
 
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterCaseLabel);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterClass);
@@ -14373,6 +14374,16 @@
   verifyFormat("auto lambda = [ = a]() { a = 2; };", AlignStyle);
 }
 
+TEST_F(FormatTest, SpacesAroundConditions) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.SpacesAroundConditions = true;
+  verifyFormat("if ( !a )\n  return;", Spaces);
+  verifyFormat("if ( a )\n  return;", Spaces);
+  verifyFormat("while ( a )\n  return;", Spaces);
+  verifyFormat("while ( (a && b) )\n  return;", Spaces);
+  verifyFormat("do {\n} while ( 1 != 0 );", Spaces);
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2505,6 +2505,18 @@
 return Right.is(tok::hash);
   if (Left.is(tok::l_paren) && Right.is(tok::r_paren))
 return Style.SpaceInEmptyParentheses;
+  if (Style.SpacesAroundConditions) {
+const auto is_cond_kw = [&](const FormatToken *t) {
+  return t->isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
+tok::kw_switch, tok::kw_constexpr, TT_ForEachMacro);
+};
+if (Left.is(tok::l_paren) && Left.Previous && is_cond_kw(Left.Previous))
+  return true;
+if (Right.is(tok::r_paren) && Right.MatchingParen &&
+Right.MatchingParen->Previous &&
+is_cond_kw(Right.MatchingParen->Previous))
+  return true;
+  }
   if (Left.is(tok::l_paren) || Right.is(tok::r_paren))
 return (Right.is(TT_CastRParen) ||
 (Left.MatchingParen && Left.MatchingParen->is(TT_CastRParen)))
@@ -2903,7 +2915,8 @@
 // The identifier might actually be a macro name such as ALWAYS_INLINE. If
 // this turns out to be too lenient, add analysis of the identifier itself.
 return Right.WhitespaceRange.getBegin() != Right.WhitespaceRange.getEnd();
-  if (Right.is(tok::coloncolon) && !Left.isOneOf(tok::l_brace, tok::comment))
+  if (Right.is(tok::coloncolon) &&
+  !Left.isOneOf(tok::l_brace, tok::comment, tok::l_paren))
 return (Left.is(TT_TemplateOpener) &&
 Style.Standard < FormatStyle::LS_Cpp11) ||
!(Left.isOneOf(tok::l_paren, tok::r_paren, tok::l_square,
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -517,6 +517,7 @@
Style.SpacesInCStyleCastParentheses);
 IO.mapOptional("SpacesInParentheses", Style.SpacesInParentheses);
 IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
+IO.mapOptional("SpacesAroundConditions", Style.SpacesAroundConditions);
 IO.mapOptional("Standard", Style.Standard);
 IO.mapOptional("StatementMacros", Style.StatementMacros);
 IO.mapOptional("TabWidth", Style.TabWidth);
@@ -775,6 +776,7 @@
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
   LLVMStyle.SpacesInAngles = false;
+  LLVMStyle.SpacesAroundConditions = false;
 
   LLVMStyle.PenaltyBreakAssignment = prec::Assignment;
   LLVMStyle.PenaltyBreakComment = 300;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1945,6 +1945,10 @@
   /// \endcode
   bool SpacesInSquareBrackets;
 
+  /// \brief If ``true``, spaces will be inserted around if/for/while (and
+  /// similar) conditions.
+  bool SpacesAroundConditions;
+
   /// Supported language standards.
   enum LanguageStandard {
 /// Use C++03-compatible syntax.
@@ -2081,6 +2085,7 @@
SpacesInCStyleCastParentheses == R.SpacesInCStyleCastParentheses &&