[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Francois Ferrand via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL326192: clang-format: use AfterControlStatement to format 
ObjC control blocks (authored by Typz, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D43232

Files:
  cfe/trunk/include/clang/Format/Format.h
  cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
  cfe/trunk/lib/Format/UnwrappedLineParser.cpp
  cfe/trunk/unittests/Format/FormatTestObjC.cpp


Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp
@@ -1129,7 +1129,7 @@
   case tok::objc_autoreleasepool:
 nextToken();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
@@ -1141,7 +1141,7 @@
// Skip synchronization object
parseParens();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
Index: cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
@@ -314,6 +314,14 @@
   }
   return MergedLines;
 }
+// Don't merge block with left brace wrapped after ObjC special blocks
+if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
+I[-1]->First->is(tok::at) && I[-1]->First->Next) {
+  tok::ObjCKeywordKind kwId = I[-1]->First->Next->Tok.getObjCKeywordID();
+  if (kwId == clang::tok::objc_autoreleasepool ||
+  kwId == clang::tok::objc_synchronized)
+return 0;
+}
 // Try to merge a block with left brace wrapped that wasn't yet covered
 if (TheLine->Last->is(tok::l_brace)) {
   return !Style.BraceWrapping.AfterFunction ||
Index: cfe/trunk/unittests/Format/FormatTestObjC.cpp
===
--- cfe/trunk/unittests/Format/FormatTestObjC.cpp
+++ cfe/trunk/unittests/Format/FormatTestObjC.cpp
@@ -187,7 +187,8 @@
"@autoreleasepool {\n"
"  f();\n"
"}\n");
-  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterControlStatement = true;
   verifyFormat("@autoreleasepool\n"
"{\n"
"  f();\n"
@@ -216,7 +217,8 @@
"@synchronized(self) {\n"
"  f();\n"
"}\n");
-  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterControlStatement = true;
   verifyFormat("@synchronized(self)\n"
"{\n"
"  f();\n"
Index: cfe/trunk/include/clang/Format/Format.h
===
--- cfe/trunk/include/clang/Format/Format.h
+++ cfe/trunk/include/clang/Format/Format.h
@@ -662,7 +662,9 @@
 ///   }
 /// \endcode
 bool AfterNamespace;
-/// \brief Wrap ObjC definitions (``@autoreleasepool``, interfaces, ..).
+/// \brief Wrap ObjC definitions (interfaces, implementations...).
+/// \note @autoreleasepool and @synchronized blocks are wrapped
+/// according to `AfterControlStatement` flag.
 bool AfterObjCDeclaration;
 /// \brief Wrap struct definitions.
 /// \code


Index: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp
@@ -1129,7 +1129,7 @@
   case tok::objc_autoreleasepool:
 nextToken();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
@@ -1141,7 +1141,7 @@
// Skip synchronization object
parseParens();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
Index: cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
===
--- cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
+++ 

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz updated this revision to Diff 134641.
Typz added a comment.

Fix bug which was lingering: prevent inlining of ObjC special control blocks.


Repository:
  rC Clang

https://reviews.llvm.org/D43232

Files:
  include/clang/Format/Format.h
  lib/Format/UnwrappedLineFormatter.cpp
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTestObjC.cpp


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -178,7 +178,8 @@
"@autoreleasepool {\n"
"  f();\n"
"}\n");
-  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterControlStatement = true;
   verifyFormat("@autoreleasepool\n"
"{\n"
"  f();\n"
@@ -196,7 +197,8 @@
"@synchronized(self) {\n"
"  f();\n"
"}\n");
-  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterControlStatement = true;
   verifyFormat("@synchronized(self)\n"
"{\n"
"  f();\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1115,7 +1115,7 @@
   case tok::objc_autoreleasepool:
 nextToken();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
@@ -1127,7 +1127,7 @@
// Skip synchronization object
parseParens();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
Index: lib/Format/UnwrappedLineFormatter.cpp
===
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -314,6 +314,14 @@
   }
   return MergedLines;
 }
+// Don't merge block with left brace wrapped after ObjC special blocks
+if (TheLine->First->is(tok::l_brace) && I != AnnotatedLines.begin() &&
+I[-1]->First->is(tok::at) && I[-1]->First->Next) {
+  tok::ObjCKeywordKind kwId = I[-1]->First->Next->Tok.getObjCKeywordID();
+  if (kwId == clang::tok::objc_autoreleasepool ||
+  kwId == clang::tok::objc_synchronized)
+return 0;
+}
 // Try to merge a block with left brace wrapped that wasn't yet covered
 if (TheLine->Last->is(tok::l_brace)) {
   return !Style.BraceWrapping.AfterFunction ||
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -651,7 +651,9 @@
 ///   }
 /// \endcode
 bool AfterNamespace;
-/// \brief Wrap ObjC definitions (``@autoreleasepool``, interfaces, ..).
+/// \brief Wrap ObjC definitions (interfaces, implementations...).
+/// \note @autoreleasepool and @synchronized blocks are wrapped
+/// according to `AfterControlStatement` flag.
 bool AfterObjCDeclaration;
 /// \brief Wrap struct definitions.
 /// \code


Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -178,7 +178,8 @@
"@autoreleasepool {\n"
"  f();\n"
"}\n");
-  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterControlStatement = true;
   verifyFormat("@autoreleasepool\n"
"{\n"
"  f();\n"
@@ -196,7 +197,8 @@
"@synchronized(self) {\n"
"  f();\n"
"}\n");
-  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterControlStatement = true;
   verifyFormat("@synchronized(self)\n"
"{\n"
"  f();\n"
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1115,7 +1115,7 @@
   case tok::objc_autoreleasepool:
 nextToken();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

I'll try to fix this.

Can you please review D43114  in the mean 
time? This patch depends on it, so it must be merged first.


Repository:
  rC Clang

https://reviews.llvm.org/D43232



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

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

> This used to happen before as well, and would require fixing 
> UnwrappedLineFormatter.cpp to understand that these blocks are linked to the 
> previous ObjC keyword.

Good find! Looks like we should fix that and add some more tests with 
AfterControlStatement set to true and to false.


Repository:
  rC Clang

https://reviews.llvm.org/D43232



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

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

In https://reviews.llvm.org/D43232#1008004, @benhamilton wrote:

> Thanks! Can you add a test for this, please?


There are actually tests, though they are performed by activating Allman brace 
wrapping mode.
I am trying to change them, but this highlights another issue: when only 
`AfterControlStatement` is set, the block gets inlined, like this:

  @autoreleasepool
  { f(); }

This used to happen before as well, and would require fixing 
UnwrappedLineFormatter.cpp to understand that these blocks are linked to the 
previous ObjC keyword.


Repository:
  rC Clang

https://reviews.llvm.org/D43232



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

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton requested changes to this revision.
benhamilton added a comment.
This revision now requires changes to proceed.

Thanks! Can you add a test for this, please?


Repository:
  rC Clang

https://reviews.llvm.org/D43232



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

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz added a comment.

This is done as discussed in https://reviews.llvm.org/D43114.
But I can instead add a new `AfterObjCSpecialBlock` brace wrapping flag.


Repository:
  rC Clang

https://reviews.llvm.org/D43232



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

[PATCH] D43232: clang-format: use AfterControlStatement to format ObjC control blocks

[Francois Ferrand via Phabricator via cfe-commits]

Typz created this revision.
Typz added reviewers: krasimir, djasper, klimek, benhamilton.

ObjC defines `@autoreleasepool` and `@synchronized` control blocks. These
used to be formatted according to the `AfterObjCDeclaration` brace-
wrapping flag, which is not very consistent.

This patch changes the behavior to use the `AfterControlStatement` flag
instead. This should not affect the behavior unless a custom brace
wrapping mode is used.


Repository:
  rC Clang

https://reviews.llvm.org/D43232

Files:
  include/clang/Format/Format.h
  lib/Format/UnwrappedLineParser.cpp


Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1115,7 +1115,7 @@
   case tok::objc_autoreleasepool:
 nextToken();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
@@ -1127,7 +1127,7 @@
// Skip synchronization object
parseParens();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -651,7 +651,9 @@
 ///   }
 /// \endcode
 bool AfterNamespace;
-/// \brief Wrap ObjC definitions (``@autoreleasepool``, interfaces, ..).
+/// \brief Wrap ObjC definitions (interfaces, implementations...).
+/// \note @autoreleasepool and @synchronized blocks are wrapped
+/// according to `AfterControlStatement` flag.
 bool AfterObjCDeclaration;
 /// \brief Wrap struct definitions.
 /// \code


Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -1115,7 +1115,7 @@
   case tok::objc_autoreleasepool:
 nextToken();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
@@ -1127,7 +1127,7 @@
// Skip synchronization object
parseParens();
 if (FormatTok->Tok.is(tok::l_brace)) {
-  if (Style.BraceWrapping.AfterObjCDeclaration)
+  if (Style.BraceWrapping.AfterControlStatement)
 addUnwrappedLine();
   parseBlock(/*MustBeDeclaration=*/false);
 }
Index: include/clang/Format/Format.h
===
--- include/clang/Format/Format.h
+++ include/clang/Format/Format.h
@@ -651,7 +651,9 @@
 ///   }
 /// \endcode
 bool AfterNamespace;
-/// \brief Wrap ObjC definitions (``@autoreleasepool``, interfaces, ..).
+/// \brief Wrap ObjC definitions (interfaces, implementations...).
+/// \note @autoreleasepool and @synchronized blocks are wrapped
+/// according to `AfterControlStatement` flag.
 bool AfterObjCDeclaration;
 /// \brief Wrap struct definitions.
 /// \code
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits