clang-format: Add two new formatting options

[rsmmr]

Summary

This patch adds two new options, feedback welcome.

SpacesAroundConditions (bool)

If true, spaces will be inserted around if/for/while conditions.

SpacesAfterNot (bool)

If true, spaces will be inserted after '!'.

[elsteveogrande]

Thought I'd take a look at the list of diffs and take a stab at a few (a few easy-looking and short ones :) ) to unblock some ppl, and unless @djasper has strong opinions about it I'd say why not.

Looking at other rules "nearby" I see spaces inside and outside square-brackets and angle-brackets, so it makes sense to allow this for conditions, especially if someone thinks this helps make it more readable, and for not as well (I personally like that one, it's easy to eyeball right past that !.

Also it appears to be off by default and I assume the unit tests work :)

[rsmmr]

Cool, thanks Steve. What's the next step for me to get it committed?

[djasper]

Could you read: http://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options
And provide some evidence about the requirements for new style options?

[rsmmr]

Sure, I'm aiming to use clang-format on a couple of open-source code bases using this style, with the main one being the Bro network security monitor, see www.bro.org and github.com/bro/bro (note the stars and forks :-) Bro is also featured on GitHub's list of security show cases, https://github.com/showcases/security.

[djasper]

Sorry, but that's actually not enough, at least at first sight. With 37 contributors total, bro is still quite small and only 12 of them have more than a handful of commits. And it doesn't have a real style guide. It has: https://www.bro.org/development/contribute.html#coding-guidelines, which basically says "adapt to the existing code structure" and "We don’t have many strict rules".

[rsmmr]

Well, last time I counted it was more like 100 contributors---Bro existed before GitHub (and before git). The style-guide is lacking, yes ... what can I say.

I don't really want to argue about importance of the project. We would like to use clang-format here and elsewhere, and I'd think these options could be useful to others as well, so I created a patch and submitted. If you guys don't like it, your call.

[djasper]

It's not about whether or not we like the patch. It's whether adding these options is a good trade-off for clang-format overall. If we find that actually more people would find these styles desirable, we can reconsider.

I have left some comments anyway in case you want to keep the patch around.

include/clang/Format/Format.h

603

It's actually more than if/for/while.

lib/Format/Format.cpp

355

Unnecessary linebreaks.

lib/Format/TokenAnnotator.cpp

1989

Indent is off.

1989–1990

This should go into a helper function or lambda so that it can be reused. What about catch? Also some of these aren't tested, e.g. the preprocessor ones.

1993

Indent is off.

2232

Note that at least JavaScript/TypeScript has a ! postfix operator, i.e. unary operator that applies to the token before it. You definitely don't want to always add a space after that.

2233

Indent is off.

2252

Is this related in any way? I don't see a test with ::

unittests/Format/FormatTest.cpp

11508

While you have added some test here, I think the more debatable ones are actually where there is also a space before the !, e.g.:

if (a && ! b) ..
  return ! c;
bool x = ! y && z;

The last one is especially tricky, because it makes it less clear that ! binds stronger than &&. Might seem obvious in this case, but IIRC, we fought quite a few bugs of the form:

if (! a < b) ..

Until a warning was added in Clang.

[shafik]

Is this still relevant or should this be closed?

docs/ClangFormatStyleOptions.rst

 **SpacesInSquareBrackets** (``bool``)
   If ``true``, spaces will be inserted after ``[`` and before ``]``.
 
+**SpacesAroundConditions** (``bool``)
+  If ``true``, spaces will be inserted around if/for/while conditions.
+
+**SpacesAfterNot** (``bool``)
+  If ``true``, spaces will be inserted after ``!``.
+
 **Standard** (``LanguageStandard``)
   Format compatible with this standard, e.g. use ``A<A<int> >``
   instead of ``A<A<int>>`` for ``LS_Cpp03``.

include/clang/Format/Format.h

   /// \brief If ``true``, spaces will be inserted after ``[`` and before ``]``.
   bool SpacesInSquareBrackets;
 
+  /// \brief If ``true``, spaces will be inserted around if/for/while conditions.
+  bool SpacesAroundConditions;
+
+  /// \brief If ``true``, spaces will be inserted after ``!``.
+  bool SpacesAfterNot;
+
   /// \brief Supported language standards.
   enum LanguageStandard {
     /// Use C++03-compatible syntax.

            SpacesInCStyleCastParentheses == R.SpacesInCStyleCastParentheses &&
            SpacesInParentheses == R.SpacesInParentheses &&
            SpacesInSquareBrackets == R.SpacesInSquareBrackets &&
+           SpacesAfterNot == R.SpacesAfterNot &&
+           SpacesAroundConditions == R.SpacesAroundConditions &&
            Standard == R.Standard && TabWidth == R.TabWidth &&
            UseTab == R.UseTab;
   }

lib/Format/Format.cpp

                    Style.SpacesInCStyleCastParentheses);
     IO.mapOptional("SpacesInParentheses", Style.SpacesInParentheses);
     IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
+    IO.mapOptional("SpacesAfterNot",
+                   Style.SpacesAfterNot);
+    IO.mapOptional("SpacesAroundConditions",
+                   Style.SpacesAroundConditions);
     IO.mapOptional("Standard", Style.Standard);
     IO.mapOptional("TabWidth", Style.TabWidth);
     IO.mapOptional("UseTab", Style.UseTab);

   LLVMStyle.SpaceBeforeParens = FormatStyle::SBPO_ControlStatements;
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpacesInAngles = false;
+  LLVMStyle.SpacesAfterNot = false;
+  LLVMStyle.SpacesAroundConditions = false;
 
   LLVMStyle.PenaltyBreakComment = 300;
   LLVMStyle.PenaltyBreakFirstLessLess = 120;

lib/Format/TokenAnnotator.cpp

     return Right.is(tok::hash);
   if (Left.is(tok::l_paren) && Right.is(tok::r_paren))
     return Style.SpaceInEmptyParentheses;
+  if (Style.SpacesAroundConditions) {
+    if (Left.is(tok::l_paren) && Left.Previous &&
+      Left.Previous->isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
+                             tok::kw_switch, TT_ForEachMacro))
+        return true;
+    if (Right.is(tok::r_paren) && Right.MatchingParen && Right.MatchingParen->Previous &&
+      Right.MatchingParen->Previous->isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
+                                             tok::kw_switch, TT_ForEachMacro))
+        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)))

       return Style.SpacesInContainerLiterals;
     return true;
   }
+ if (Style.SpacesAfterNot && Left.is(tok::exclaim))
+        return true;
   if (Left.is(TT_UnaryOperator))
     return Right.is(TT_BinaryOperator);
 

   if (!Style.SpaceBeforeAssignmentOperators &&
       Right.getPrecedence() == prec::Assignment)
     return false;
-  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_Cpp03) ||
            !(Left.isOneOf(tok::identifier, tok::l_paren, tok::r_paren,

unittests/Format/FormatTest.cpp

   CHECK_PARSE_BOOL(SpaceAfterCStyleCast);
   CHECK_PARSE_BOOL(SpaceAfterTemplateKeyword);
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
+  CHECK_PARSE_BOOL(SpacesAfterNot);
+  CHECK_PARSE_BOOL(SpacesAroundConditions);
 
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterClass);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterControlStatement);

   verifyFormat("include \"a.td\"\ninclude \"b.td\"", Style);
 }
 
+TEST_F(FormatTest, SpacesAfterNot) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.SpacesAfterNot = true;
+  verifyFormat("if (! a)\n  return;", Spaces);
+  verifyFormat("while (! (x || y))\n  return;", Spaces);
+  verifyFormat("do {\n} while (! foo);", Spaces);
+}
+
+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);
+}
+
 // Since this test case uses UNIX-style file path. We disable it for MS
 // compiler.
 #if !defined(_MSC_VER) && !defined(__MINGW32__)