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

[timwoj]

Summary

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)

[MyDeveloperDay]

clang/include/clang/Format/Format.h

2000

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

2172

move upto just before SpacesBeforeTrailingComments

clang/lib/Format/Format.cpp

548

Same here

822

Sorry move to line 771-772

clang/lib/Format/TokenAnnotator.cpp

2621

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(....)

3064

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

[mitchell-stellar]

clang/docs/ClangFormatStyleOptions.rst

2363

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.

clang/lib/Format/TokenAnnotator.cpp

2623

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

3064

This is not covered by your tests.

[timwoj]

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

[timwoj]

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.

[mitchell-stellar]

clang/include/clang/Format/Format.h

2005

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

[timwoj]

Fix ordering

[mitchell-stellar]

LGTM

[MyDeveloperDay]

LGTM

[MyDeveloperDay]

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 bugs 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

[timwoj]

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?

[MyDeveloperDay]

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

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

14889

show "else if" example

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

14893

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

clang/docs/ClangFormatStyleOptions.rst

 **SpacesInCStyleCastParentheses** (``bool``)
   If ``true``, spaces may be inserted into C style casts.
 
   .. code-block:: c++
 
      true:                                  false:
      x = ( int32 )y                 vs.     x = (int32)y
 
+**SpacesInConditionalStatement** (``bool``)
+  If ``true``, spaces will be inserted around if/for/while (and similar) conditions.
+
 **SpacesInContainerLiterals** (``bool``)
   If ``true``, spaces are inserted inside container literals (e.g.
   ObjC and Javascript array and dict literals).
 
   .. code-block:: js
 
      true:                                  false:
      var arr = [ 1, 2, 3 ];         vs.     var arr = [1, 2, 3];
**SpacesInSquareBrackets** (``bool``)
   be affected.
 
   .. code-block:: c++
 
      true:                                  false:
      int a[ 5 ];                    vs.     int a[5];
      std::unique_ptr<int[]> foo() {} // Won't be affected
 
 **Standard** (``LanguageStandard``)
   Parse and format C++ constructs compatible with this standard.
 
   .. code-block:: c++
 
      c++03:                                 latest:
      vector<set<int> > x;           vs.     vector<set<int>> x;
 
   Possible values:

clang/include/clang/Format/Format.h

   /// in template argument lists.
   /// \code
   ///    true:                                  false:
   ///    static_cast< int >(arg);       vs.     static_cast<int>(arg);
   ///    std::function< void(int) > fct;        std::function<void(int)> fct;
   /// \endcode
   bool SpacesInAngles;
 
+  /// If ``true``, spaces will be inserted around if/for/switch/while
+  /// conditions.
+  /// \code
+  ///    true:                                  false:
+  ///    if ( a )  { ... }              vs.     if (a) { ... }
+  ///    while ( i < 5 )  { ... }               while (i < 5) { ... }
+  /// \endcode
+  bool SpacesInConditionalStatement;
+
   /// If ``true``, spaces are inserted inside container literals (e.g.
   /// ObjC and Javascript array and dict literals).
   /// \code{.js}
   ///    true:                                  false:
   ///    var arr = [ 1, 2, 3 ];         vs.     var arr = [1, 2, 3];
   ///    f({a : 1, b : 2, c : 3});              f({a: 1, b: 2, c: 3});
   /// \endcode
   bool SpacesInContainerLiterals;

   ///    true:                                  false:
   ///    int a[ 5 ];                    vs.     int a[5];
   ///    std::unique_ptr<int[]> foo() {} // Won't be affected
   /// \endcode
   bool SpacesInSquareBrackets;
 
   /// If ``true``, spaces will be before  ``[``.
   /// Lambdas will not be affected. Only the first ``[`` will get a space added.
   /// \code
   ///    true:                                  false:
   ///    int a [5];                    vs.      int a[5];
   ///    int a [5][5];                 vs.      int a[5][5];
   /// \endcode
   bool SpaceBeforeSquareBrackets;
 
   /// Supported language standards for parsing and formatting C++ constructs.
   /// \code
   ///    Latest:                                vector<set<int>>
   ///    c++03                          vs.     vector<set<int> >
   /// \endcode
   ///
   /// The correct way to spell a specific language version is e.g. ``c++11``.
return AccessModifierOffset == R.AccessModifierOffset &&
            SpaceBeforeInheritanceColon == R.SpaceBeforeInheritanceColon &&
            SpaceBeforeParens == R.SpaceBeforeParens &&
            SpaceBeforeRangeBasedForLoopColon ==
                R.SpaceBeforeRangeBasedForLoopColon &&
            SpaceInEmptyBlock == R.SpaceInEmptyBlock &&
            SpaceInEmptyParentheses == R.SpaceInEmptyParentheses &&
            SpacesBeforeTrailingComments == R.SpacesBeforeTrailingComments &&
            SpacesInAngles == R.SpacesInAngles &&
+           SpacesInConditionalStatement == R.SpacesInConditionalStatement &&
            SpacesInContainerLiterals == R.SpacesInContainerLiterals &&
            SpacesInCStyleCastParentheses == R.SpacesInCStyleCastParentheses &&
            SpacesInParentheses == R.SpacesInParentheses &&
            SpacesInSquareBrackets == R.SpacesInSquareBrackets &&
            SpaceBeforeSquareBrackets == R.SpaceBeforeSquareBrackets &&
            Standard == R.Standard && TabWidth == R.TabWidth &&
            StatementMacros == R.StatementMacros && UseTab == R.UseTab &&
            UseCRLF == R.UseCRLF && TypenameMacros == R.TypenameMacros;
   }
 
   llvm::Optional<FormatStyle> GetLanguageStyle(LanguageKind Language) const;
 
   // Stores per-language styles. A FormatStyle instance inside has an empty

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
     IO.mapOptional("SpaceBeforeRangeBasedForLoopColon",
                    Style.SpaceBeforeRangeBasedForLoopColon);
     IO.mapOptional("SpaceInEmptyBlock", Style.SpaceInEmptyBlock);
     IO.mapOptional("SpaceInEmptyParentheses", Style.SpaceInEmptyParentheses);
     IO.mapOptional("SpacesBeforeTrailingComments",
                    Style.SpacesBeforeTrailingComments);
     IO.mapOptional("SpacesInAngles", Style.SpacesInAngles);
+    IO.mapOptional("SpacesInConditionalStatement",
+                   Style.SpacesInConditionalStatement);
     IO.mapOptional("SpacesInContainerLiterals",
                    Style.SpacesInContainerLiterals);
     IO.mapOptional("SpacesInCStyleCastParentheses",
                    Style.SpacesInCStyleCastParentheses);
     IO.mapOptional("SpacesInParentheses", Style.SpacesInParentheses);
     IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
     IO.mapOptional("SpaceBeforeSquareBrackets",
                    Style.SpaceBeforeSquareBrackets);
     IO.mapOptional("Standard", Style.Standard);
     IO.mapOptional("StatementMacros", Style.StatementMacros);
     IO.mapOptional("TabWidth", Style.TabWidth);
     IO.mapOptional("TypenameMacros", Style.TypenameMacros);
     IO.mapOptional("UseCRLF", Style.UseCRLF);
     IO.mapOptional("UseTab", Style.UseTab);
   }
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.SpaceBeforeCtorInitializerColon = true;
   LLVMStyle.SpaceBeforeInheritanceColon = true;
   LLVMStyle.SpaceBeforeParens = FormatStyle::SBPO_ControlStatements;
   LLVMStyle.SpaceBeforeRangeBasedForLoopColon = true;
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
   LLVMStyle.SpaceBeforeSquareBrackets = false;
   LLVMStyle.SpacesInAngles = false;
+  LLVMStyle.SpacesInConditionalStatement = false;
 
   LLVMStyle.PenaltyBreakAssignment = prec::Assignment;
   LLVMStyle.PenaltyBreakComment = 300;
   LLVMStyle.PenaltyBreakFirstLessLess = 120;
   LLVMStyle.PenaltyBreakString = 1000;
   LLVMStyle.PenaltyExcessCharacter = 1000000;
   LLVMStyle.PenaltyReturnTypeOnItsOwnLine = 60;
   LLVMStyle.PenaltyBreakBeforeFirstCallParameter = 19;

clang/lib/Format/TokenAnnotator.cpp

 }
 
 bool TokenAnnotator::spaceRequiredBeforeParens(const FormatToken &Right) const {
   return Style.SpaceBeforeParens == FormatStyle::SBPO_Always ||
          (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
           Right.ParameterCount > 0);
 }
 
+/// Returns \c true if the token is followed by a boolean condition, \c false
+/// otherwise.
+static bool isKeywordWithCondition(const FormatToken &Tok) {
+  return Tok.isOneOf(tok::kw_if, tok::kw_for, tok::kw_while, tok::kw_switch,
+                     tok::kw_constexpr);
+};
+
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
                                           const FormatToken &Left,
                                           const FormatToken &Right) {
   if (Left.is(tok::kw_return) && Right.isNot(tok::semi))
     return true;
   if (Left.is(Keywords.kw_assert) && Style.Language == FormatStyle::LK_Java)
     return true;
   if (Style.ObjCSpaceAfterProperty && Line.Type == LT_ObjCProperty &&
       Left.Tok.getObjCKeywordID() == tok::objc_property)
     return true;
   if (Right.is(tok::hashhash))
     return Left.is(tok::hash);
   if (Left.isOneOf(tok::hashhash, tok::hash))
     return Right.is(tok::hash);
   if ((Left.is(tok::l_paren) && Right.is(tok::r_paren)) ||
       (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)))
                ? Style.SpacesInCStyleCastParentheses
                : Style.SpacesInParentheses;
   if (Right.isOneOf(tok::semi, tok::comma))
     return false;
   if (Right.is(tok::less) && Line.Type == LT_ObjCDecl) {
bool TokenAnnotator::spaceRequiredBefore(const AnnotatedLine &Line,
   if (Style.Language == FormatStyle::LK_Java && Right.is(tok::coloncolon) &&
       (Left.is(tok::identifier) || Left.is(tok::kw_this)))
     return false;
   if (Right.is(tok::coloncolon) && Left.is(tok::identifier))
     // Generally don't remove existing spaces between an identifier and "::".
     // 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,
                           tok::kw___super, TT_TemplateCloser,
                           TT_TemplateOpener)) ||
            (Left.is(tok ::l_paren) && Style.SpacesInParentheses);
   if ((Left.is(TT_TemplateOpener)) != (Right.is(TT_TemplateCloser)))
     return Style.SpacesInAngles;

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(ObjCSpaceBeforeProtocolList);
   CHECK_PARSE_BOOL(Cpp11BracedListStyle);
   CHECK_PARSE_BOOL(ReflowComments);
   CHECK_PARSE_BOOL(SortIncludes);
   CHECK_PARSE_BOOL(SortUsingDeclarations);
   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);
   CHECK_PARSE_BOOL(SpacesInCStyleCastParentheses);
   CHECK_PARSE_BOOL(SpaceAfterCStyleCast);
   CHECK_PARSE_BOOL(SpaceAfterTemplateKeyword);
   CHECK_PARSE_BOOL(SpaceAfterLogicalNot);
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
TEST_F(FormatTest, AmbersandInLamda) {
   // Test case reported in https://bugs.llvm.org/show_bug.cgi?id=41899
   FormatStyle AlignStyle = getLLVMStyle();
   AlignStyle.PointerAlignment = FormatStyle::PAS_Left;
   verifyFormat("auto lambda = [&a = a]() { a = 2; };", AlignStyle);
   AlignStyle.PointerAlignment = FormatStyle::PAS_Right;
   verifyFormat("auto lambda = [&a = 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.
   verifyFormat("int a and b;");
   verifyFormat("int a and_eq b;");
   verifyFormat("int a bitand b;");
   verifyFormat("int a bitor b;");
   verifyFormat("int a compl b;");