[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

1950

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

2088

move upto just before SpacesBeforeTrailingComments

clang/lib/Format/Format.cpp

520

Same here

779

Sorry move to line 771-772

clang/lib/Format/TokenAnnotator.cpp

2509

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

2919

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

2290

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

2511

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

2919

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

1955

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

14382

show "else if" example

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

14386

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

clang/docs/ClangFormatStyleOptions.rst

**SpacesInSquareBrackets** (``bool``)
   Lambdas or unspecified size array declarations will not be affected.
 
   .. code-block:: c++
 
      true:                                  false:
      int a[ 5 ];                    vs.     int a[5];
      std::unique_ptr<int[]> foo() {} // Won't be affected
 
+**SpacesAroundConditions** (``bool``)
+  If ``true``, spaces will be inserted around if/for/while (and similar) conditions.
+
 **Standard** (``LanguageStandard``)
   Format compatible with this standard, e.g. use ``A<A<int> >``
   instead of ``A<A<int>>`` for ``LS_Cpp03``.
 
   Possible values:
 
   * ``LS_Cpp03`` (in configuration: ``Cpp03``)
     Use C++03-compatible syntax.

clang/include/clang/Format/Format.h

   /// Lambdas or unspecified size array declarations will not be affected.
   /// \code
   ///    true:                                  false:
   ///    int a[ 5 ];                    vs.     int a[5];
   ///    std::unique_ptr<int[]> foo() {} // Won't be affected
   /// \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.
     LS_Cpp03,
     /// Use features of C++11, C++14 and C++1z (e.g. ``A<A<int>>`` instead of
     /// ``A<A<int> >``).
     LS_Cpp11,
     /// Automatic detection based on the input.
     LS_Auto
   };
 
   /// Format compatible with this standard, e.g. use ``A<A<int> >``
return AccessModifierOffset == R.AccessModifierOffset &&
            SpaceInEmptyBlock == R.SpaceInEmptyBlock &&
            SpaceInEmptyParentheses == R.SpaceInEmptyParentheses &&
            SpacesBeforeTrailingComments == R.SpacesBeforeTrailingComments &&
            SpacesInAngles == R.SpacesInAngles &&
            SpacesInContainerLiterals == R.SpacesInContainerLiterals &&
            SpacesInCStyleCastParentheses == R.SpacesInCStyleCastParentheses &&
            SpacesInParentheses == R.SpacesInParentheses &&
            SpacesInSquareBrackets == R.SpacesInSquareBrackets &&
+           SpacesAroundConditions == R.SpacesAroundConditions &&
            Standard == R.Standard && TabWidth == R.TabWidth &&
            StatementMacros == R.StatementMacros && UseTab == R.UseTab &&
            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

IO.mapOptional("SpacesBeforeTrailingComments",
                    Style.SpacesBeforeTrailingComments);
     IO.mapOptional("SpacesInAngles", Style.SpacesInAngles);
     IO.mapOptional("SpacesInContainerLiterals",
                    Style.SpacesInContainerLiterals);
     IO.mapOptional("SpacesInCStyleCastParentheses",
                    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);
     IO.mapOptional("TypenameMacros", Style.TypenameMacros);
     IO.mapOptional("UseTab", Style.UseTab);
   }
 };
 
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.SpaceAfterTemplateKeyword = true;
   LLVMStyle.SpaceBeforeCtorInitializerColon = true;
   LLVMStyle.SpaceBeforeInheritanceColon = true;
   LLVMStyle.SpaceBeforeParens = FormatStyle::SBPO_ControlStatements;
   LLVMStyle.SpaceBeforeRangeBasedForLoopColon = true;
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
   LLVMStyle.SpacesInAngles = false;
+  LLVMStyle.SpacesAroundConditions = 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

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))
     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)))
                ? 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(SpaceAfterCStyleCast);
   CHECK_PARSE_BOOL(SpaceAfterTemplateKeyword);
   CHECK_PARSE_BOOL(SpaceAfterLogicalNot);
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
   CHECK_PARSE_BOOL(SpaceBeforeCpp11BracedList);
   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);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterControlStatement);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterEnum);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterFunction);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterNamespace);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterObjCDeclaration);
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, 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