clang-format: Support @synchronized.

[strager]

Summary

Format @synchronized properly for the Attach brace style by
recognizing @synchronized similar to 'while'. Doing this
triggers the SpaceBeforeParens rule, so fix up existing
tests which assumed the old behaviour.

Depends on D10420.

[djasper]

I can't see the changes to tests at the moment. Trying to fix that issue with SVN/Phabricator (it's unrelated to you patch).

docs/ClangFormatStyleOptions.rst

422–424 ↗

(On Diff #27472)

Don't write these by hand. They should be auto-generated with cfe/docs/tools/dump_format_style.py.

include/clang/Format/Format.h

405 ↗

(On Diff #27472)

This should go together with ObjCSpaceAfterProperty.

496 ↗

(On Diff #27472)

Sort this to the right place.

lib/Format/FormatToken.h

287 ↗

(On Diff #27472)

remove clang::

Also, do you mind cleaning up the other usage of getObjCKeywordID() ==. There shouldn't be that many, so this can be done in the same patch.

lib/Format/TokenAnnotator.cpp

1910–1911

Can you swap the operand order? I would find that easier to read.

[strager]

docs/ClangFormatStyleOptions.rst

422–424 ↗

(On Diff #27472)

Good to know. I assume that applies for my other diffs as well.

include/clang/Format/Format.h

405 ↗

(On Diff #27472)

Yup. (I originally named it SpaceAfterObjCSynchronized, which is why it's placed here.)

lib/Format/FormatToken.h

287 ↗

(On Diff #27472)

Sure. I'll do it in a separate patch, along with similar types like preprocessor keywords.

lib/Format/TokenAnnotator.cpp

1910–1911

You mean:

`
(Left.is(tok::objc_synchronized) &&
 Style.ObjCSpaceAfterSynchronized)
`

?

I was keeping consistent with lines 1852 and 1844, where the Style check comes first.

[djasper]

docs/ClangFormatStyleOptions.rst

422–424 ↗

(On Diff #27472)

Yes.

lib/Format/FormatToken.h

287 ↗

(On Diff #27472)

AFAICT, there are only three other places in clang-format that could make use of this. Please do that in this patch. Otherwise, we'll just forget.

lib/Format/TokenAnnotator.cpp

1910–1911

It is what I meant, but you are right, keeping consistent with the rest of this (awful ;-) ) expression is probably better.

My thinking is that I like to read it better the other way round, but having the style flag first can actually bring a performance improvement for the other parts of this expression. So just leave as is.

[strager]

Fix some things.

[strager]

Fix test diff on Phabricator.

[djasper]

docs/ClangFormatStyleOptions.rst

420 ↗

(On Diff #27605)

Actually, I don't think we should have this option at all (and possibly not the one above either, but I can try to deprecate that later). Would it work for you to bind this to what is selected for SpaceBeforeParens? Specifically, can we handle this like control statements? If not, can we extend SpacesBeforeParens to have more enum values?

lib/Format/UnwrappedLineParser.cpp

683

Remove "Tok." (and below).

[strager]

I'll remove ObjCSpaceAfterSynchronized from this diff.

docs/ClangFormatStyleOptions.rst

420 ↗

(On Diff #27605)

Would it work for you to bind this to what is selected for SpaceBeforeParens? Specifically, can we handle this like control statements?

Yes. This patch does that *and* has an option to force spaces. I can split the patch in two so it's obvious. This will cause @synchronized to change its formatting, though.

Would it work for you to bind this to what is selected for SpaceBeforeParens?

Sadly, my code base doesn't work this way. =[

@synchronized(foo) {  // No space.
    bar;
}
if (baz) {  // Space.
    qix;
}

My code base seems to consistently hug ( with Objective-C @ keywords (@property(...), @synchronized(...), etc.).

If not, can we extend SpacesBeforeParens to have more enum values?

Yeah, I think that's a better approach. I'll leave that discussion for a separate patch.

[strager]

Strip out ObjCSpaceAfterSynchronized option.

[strager]

Fix commit message.

[thakis]

unittests/Format/FormatTest.cpp

7490

Hm, this looks like a bit of a regression. I've never seen @synchronized() with a space before the ( – https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Multithreading/ThreadSafety/ThreadSafety.html puts spaces after the if but not after @synchronized for example.

Maybe spaceRequiredBetween() could always return false if Right is '(' and the token before Left is @?

I'm not sure this even needs an option.

[strager]

unittests/Format/FormatTest.cpp

7490

I've never seen @synchronized() with a space before the (

I view @synchronized similar to while.

D10760 implements the behaviour you're talking about as an option.

I can treat @synchronized as something different than a ControlStatement, if that works for you.

[strager]

Rebase.

[strager]

ping

[strager]

Rebase. Fix style issues.

lib/Format/TokenAnnotator.cpp

if (CurrentToken->is(tok::caret)) {
           MaybeSel->Previous->is(tok::at)) {
         StartsObjCMethodExpr = true;
       }
     }
 
     if (Left->Previous &&
         (Left->Previous->isOneOf(tok::kw_static_assert, tok::kw_decltype,
                                  tok::kw_if, tok::kw_while, tok::l_paren,
-                                 tok::comma) ||
+                                 tok::comma, Keywords.kw_synchronized) ||
          Left->Previous->is(TT_BinaryOperator))) {
       // static_assert, if and while usually contain expressions.
       Contexts.back().IsExpression = true;
     } else if (Left->Previous && Left->Previous->is(tok::r_square) &&
                Left->Previous->MatchingParen &&
                Left->Previous->MatchingParen->is(TT_LambdaLSquare)) {
       // This is a parameter list of a lambda expression.
       Contexts.back().IsExpression = false;
if (Left.is(tok::r_paren) && Left.is(TT_AttributeParen))
       return true;
     return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
            (Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
             (Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
                           tok::kw_switch, tok::kw_case, TT_ForEachMacro) ||
              (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
                            tok::kw_new, tok::kw_delete) &&
               (!Left.Previous || Left.Previous->isNot(tok::period))))) ||
            (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
             (Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
              Left.is(tok::r_paren)) &&
             Line.Type != LT_PreprocessorDirective);
   }
   if (Left.is(tok::at) && Right.Tok.getObjCKeywordID() != tok::objc_not_keyword)
     return false;
   if (Right.is(TT_UnaryOperator))
     return !Left.isOneOf(tok::l_paren, tok::l_square, tok::at) &&
            (Left.isNot(tok::colon) || Left.isNot(TT_ObjCMethodExpr));

lib/Format/UnwrappedLineParser.cpp

case tok::at:
     case tok::objc_end:
       return; // Handled by the caller.
     case tok::objc_optional:
     case tok::objc_required:
       nextToken();
       addUnwrappedLine();
       return;
     case tok::objc_autoreleasepool:
+    case tok::objc_synchronized:
       nextToken();
-      if (FormatTok->Tok.is(tok::l_brace)) {
+      if (FormatTok->is(tok::l_paren)) {
+        parseParens();
+      }
+      if (FormatTok->is(tok::l_brace)) {
         if (Style.BreakBeforeBraces == FormatStyle::BS_Allman ||
             Style.BreakBeforeBraces == FormatStyle::BS_GNU)
           addUnwrappedLine();
         parseBlock(/*MustBeDeclaration=*/false);
       }
       addUnwrappedLine();
       return;
     case tok::objc_try:

unittests/Format/FormatTest.cpp

verifyFormat("@autoreleasepool\n"
                "}\n"
                "@autoreleasepool\n"
                "{\n"
                "  f();\n"
                "}\n",
                Style);
 }
 
+TEST_F(FormatTest, FormatObjCSynchronized) {
+  FormatStyle Style = getLLVMStyleWithColumns(32);
+  verifyFormat("@synchronized(foo) {\n"
+               "  f();\n"
+               "}\n",
+               Style);
+  verifyFormat("@synchronized([self\n"
+               "    veryLongMethodNameWithParameters:\n"
+               "        YES]) {\n"
+               "  f();\n"
+               "}\n",
+               Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  verifyFormat("@synchronized(foo)\n"
+               "{\n"
+               "  f();\n"
+               "}\n"
+               "@synchronized(foo)\n"
+               "{\n"
+               "  f();\n"
+               "}\n",
+               Style);
+}
+
 TEST_F(FormatTest, StaticInitializers) {
   verifyFormat("static SomeClass SC = {1, 'a'};");
 
   verifyFormat("static SomeClass WithALoooooooooooooooooooongName = {\n"
                "    100000000, "
                "\"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"};");
 
   // Here, everything other than the "}" would fit on a line.
TEST_F(FormatTest, ObjCSnippets) {
   verifyFormat("@class Foo, Bar;");
   verifyFormat("@compatibility_alias AliasName ExistingClass;");
   verifyFormat("@dynamic textColor;");
   verifyFormat("char *buf1 = @encode(int *);");
   verifyFormat("char *buf1 = @encode(typeof(4 * 5));");
   verifyFormat("char *buf1 = @encode(int **);");
   verifyFormat("Protocol *proto = @protocol(p1);");
   verifyFormat("SEL s = @selector(foo:);");
   verifyFormat("@synchronized(self) {\n"
                "  f();\n"
                "}");
 
   verifyFormat("@synthesize dropArrowPosition = dropArrowPosition_;");
   verifyGoogleFormat("@synthesize dropArrowPosition = dropArrowPosition_;");
 
   verifyFormat("@property(assign, nonatomic) CGFloat hoverAlpha;");
   verifyFormat("@property(assign, getter=isEditable) BOOL editable;");
verifyFormat("do {\n"
                "  do_something();\n"
                "} while(something());",
                NoSpace);
   verifyFormat("switch(x) {\n"
                "default:\n"
                "  break;\n"
                "}",
                NoSpace);
+  verifyFormat("@synchronized(x) {\n"
+               "  do_something();\n"
+               "}",
+               NoSpace);
   verifyFormat("auto i = std::make_unique<int>(5);", NoSpace);
   verifyFormat("size_t x = sizeof(x);", NoSpace);
   verifyFormat("auto f(int x) -> decltype(x);", NoSpace);
   verifyFormat("int f(T x) noexcept(x.create());", NoSpace);
   verifyFormat("alignas(128) char a[128];", NoSpace);
   verifyFormat("size_t x = alignof(MyType);", NoSpace);
   verifyFormat("static_assert(sizeof(char) == 1, \"Impossible!\");", NoSpace);
   verifyFormat("int f() throw(Deprecated);", NoSpace);
verifyFormat("do {\n"
                "  do_something ();\n"
                "} while (something ());",
                Space);
   verifyFormat("switch (x) {\n"
                "default:\n"
                "  break;\n"
                "}",
                Space);
+  verifyFormat("@synchronized (x) {\n"
+               "  do_something ();\n"
+               "}",
+               Space);
   verifyFormat("A::A () : a (1) {}", Space);
   verifyFormat("void f () __attribute__ ((asdf));", Space);
   verifyFormat("*(&a + 1);\n"
                "&((&a)[1]);\n"
                "a[(b + c) * d];\n"
                "(((a + 1) * 2) + 3) * 4;",
                Space);
   verifyFormat("#define A(x) x", Space);
verifyFormat("do {\n"
                "  do_something( (int)i );\n"
                "} while ( something() );",
                Spaces);
   verifyFormat("switch ( x ) {\n"
                "default:\n"
                "  break;\n"
                "}",
                Spaces);
+  verifyFormat("@synchronized( x ) {\n"
+               "  do_something();\n"
+               "}",
+               Spaces);
 
   Spaces.SpacesInParentheses = false;
   Spaces.SpacesInCStyleCastParentheses = true;
   verifyFormat("Type *A = ( Type * )P;", Spaces);
   verifyFormat("Type *A = ( vector<Type *, int *> )P;", Spaces);
   verifyFormat("x = ( int32 )y;", Spaces);
   verifyFormat("int a = ( int )(2.0f);", Spaces);
   verifyFormat("#define AA(X) sizeof((( X * )NULL)->a)", Spaces);
verifyFormat("do {\n"
                "  do_something(( int )i);\n"
                "} while (something( ));",
                Spaces);
   verifyFormat("switch (x) {\n"
                "default:\n"
                "  break;\n"
                "}",
                Spaces);
+  verifyFormat("@synchronized(x) {\n"
+               "  do_something( );\n"
+               "}",
+               Spaces);
 
   // Run the first set of tests again with:
   Spaces.SpaceAfterCStyleCast = true;
   verifyFormat("call(x, y, z);", Spaces);
   verifyFormat("call( );", Spaces);
   verifyFormat("std::function<void(int, int)> callback;", Spaces);
   verifyFormat("while (( bool ) 1)\n"
                "  continue;",
verifyFormat("do {\n"
                "  do_something(( int ) i);\n"
                "} while (something( ));",
                Spaces);
   verifyFormat("switch (x) {\n"
                "default:\n"
                "  break;\n"
                "}",
                Spaces);
+  verifyFormat("@synchronized(x) {\n"
+               "  do_something( );\n"
+               "}",
+               Spaces);
 
   // Run subset of tests again with:
   Spaces.SpacesInCStyleCastParentheses = false;
   Spaces.SpaceAfterCStyleCast = true;
   verifyFormat("while ((bool) 1)\n"
                "  continue;",
                Spaces);
   verifyFormat("do {\n"
                "  do_something((int) i);\n"
                "} while (something( ));",
                Spaces);
+  verifyFormat("@synchronized((NSLock) x) {\n"
+               "  do_something( );\n"
+               "}",
+               Spaces);
 }
 
 TEST_F(FormatTest, ConfigurableSpacesInSquareBrackets) {
   verifyFormat("int a[5];");
   verifyFormat("a[3] += 42;");
 
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInSquareBrackets = true;