[PATCH] D11851: clang-format: Add "AllowShortNamespacesOnASingleLine" option

2015-08-11 Thread Chris Beck via cfe-commits
cbeck88 created this revision.
cbeck88 added a reviewer: djasper.
cbeck88 added a subscriber: cfe-commits.
cbeck88 set the repository for this revision to rL LLVM.
Herald added a subscriber: klimek.

Rationale:

I sometimes use a different clang tool, iwyu ("include what you use"), to clean 
up header file inclusions in my C++ projects. Iwyu seeks to correct the 
includes of a header or cpp unit so that definitions which are needed are 
included, and definitions which only need to be forward declared are forward 
declared. It often generates code like this at the top of your file:

namespace foo { class bar; }
namespace baz { struct quaz; }
...

Unfortunately, clang-format dislikes braces which are arranged this way and 
always wants to break after them, and after the forward declaration, no matter 
what configuration options are used (as far as I can tell).

I wrote this small patch so that short namespaces like these can be set on a 
single line regardless of chosen brace-style if a boolean option 
"AllowShortNamespacesOnASingleLine" is enabled.

Repository:
  rL LLVM

http://reviews.llvm.org/D11851

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10433,6 +10433,17 @@
   verifyNoCrash("#define a\\\n /**/}");
 }
 
+TEST_F(FormatTest, ShortNamespacesOption) {
+  FormatStyle style = getLLVMStyle();
+  style.AllowShortNamespacesOnASingleLine = true;
+
+  verifyFormat("namespace foo { class bar; }", style);
+  verifyFormat("namespace foo {\nclass bar;\nclass baz;\n}", style);
+  verifyFormat("namespace foo {\nint f() { return 5; }\n}", style);
+  verifyFormat("namespace foo { template  struct bar; }", style);
+  verifyFormat("namespace foo { constexpr int num = 42; }", style);
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/UnwrappedLineFormatter.cpp
===
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -199,6 +199,12 @@
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
 if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+ TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit)) {
+  return result;
+}
+  }
   return Style.BreakBeforeBraces == FormatStyle::BS_Attach
  ? tryMergeSimpleBlock(I, E, Limit)
  : 0;
@@ -258,6 +264,35 @@
 return 1;
   }
 
+  unsigned tryMergeNamespace(
+  SmallVectorImpl::const_iterator I,
+  SmallVectorImpl::const_iterator E, unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline))
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (1 + I[1]->Last->TotalLength > Limit)
+  return 0;
+
+// An assumption of the function is that the first line begins with
+// keyword namespace (i.e. I[0]->First->is(tok::kw_namespace))
+
+// The line which is in the namespace should end with semicolon
+if (I[1]->Last->isNot(tok::semi))
+  return 0;
+
+// Last, check that the third line starts with a closing brace.
+if (I[2]->First->isNot(tok::r_brace))
+  return 0;
+
+// If so, merge all three lines.
+return 2;
+  }
+
   unsigned tryMergeSimpleControlStatement(
   SmallVectorImpl::const_iterator I,
   SmallVectorImpl::const_iterator E, unsigned Limit) {
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -212,6 +212,8 @@
Style.AllowShortIfStatementsOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -354,6 +356,7 @@
   LLVMStyle.AllowShortCaseLabelsOnASingleLine = false;
   LLVMStyle.AllowShortIfStatementsOnASingleLine = false;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AlwaysBreakTemplateDeclarations = false;
Index: include/clang/Format/Format.h
==

Re: [PATCH] D11851: clang-format: Add "AllowShortNamespacesOnASingleLine" option

2015-08-16 Thread Chris Beck via cfe-commits
cbeck88 removed rL LLVM as the repository for this revision.
cbeck88 updated this revision to Diff 32249.
cbeck88 added a comment.

- Fixed formatting in header
- Fixed handling of nested namespaces
- Added unit tests

djasper: I thought about it, I guess I don't have a particularly good reason 
that the namespaces logic should be a separate function from 
"tryMergeSimpleBlock", and maybe it should indeed be refactored to be part of 
the case analysis of that function. When I began I thought this would just be 
simpler and make it easier to customize the namespace handling vs. other 
blocks, but now I'm less sure of this. At least with the way I'm now trying to 
handle the nested namespaces, this seems a bit simpler, but maybe that part 
should be changed.


http://reviews.llvm.org/D11851

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -3226,16 +3226,19 @@
   Style.MacroBlockEnd = "^[A-Z_]+_END$";
   verifyFormat("FOO_BEGIN\n"
"  FOO_ENTRY\n"
-   "FOO_END", Style);
+   "FOO_END",
+   Style);
   verifyFormat("FOO_BEGIN\n"
"  NESTED_FOO_BEGIN\n"
"NESTED_FOO_ENTRY\n"
"  NESTED_FOO_END\n"
-   "FOO_END", Style);
+   "FOO_END",
+   Style);
   verifyFormat("FOO_BEGIN(Foo, Bar)\n"
"  int x;\n"
"  x = 1;\n"
-   "FOO_END(Baz)", Style);
+   "FOO_END(Baz)",
+   Style);
 }
 
 //===--===//
@@ -7649,14 +7652,13 @@
 
   // Verify that splitting the strings understands
   // Style::AlwaysBreakBeforeMultilineStrings.
-  EXPECT_EQ(
-  "(\n"
-  "\"aa aa \"\n"
-  "\"aa aa\");",
-  format("(\"aa "
- "aa aa "
- "aa\");",
- getGoogleStyle()));
+  EXPECT_EQ("(\n"
+"\"aa aa \"\n"
+"\"aa aa\");",
+format("(\"aa "
+   "aa aa "
+   "aa\");",
+   getGoogleStyle()));
   EXPECT_EQ("return \"aa aa \"\n"
 "   \"aa aa\";",
 format("return \"aa "
@@ -10433,6 +10435,45 @@
   verifyNoCrash("#define a\\\n /**/}");
 }
 
+TEST_F(FormatTest, ShortNamespacesOption) {
+  FormatStyle style = getLLVMStyle();
+  style.AllowShortNamespacesOnASingleLine = true;
+
+  verifyFormat("namespace foo { class bar; }", style);
+  verifyFormat("namespace foo {\n"
+   "class bar;\n"
+   "class baz;\n"
+   "}",
+   style);
+  verifyFormat("namespace foo { namespace bar { class baz; } }", style);
+  verifyFormat("namespace foo {\n"
+   "namespace bar { class baz; }\n"
+   "namespace quar { class quaz; }\n"
+   "}",
+   style);
+  verifyFormat("namespace foo {\n"
+   "int f() { return 5; }\n"
+   "}",
+   style);
+  verifyFormat("namespace foo { template  struct bar; }", style);
+  verifyFormat("namespace foo { constexpr int num = 42; }", style);
+
+  // test column limit
+  verifyFormat("namespace boost {\n"
+   "namespace spirit {\n"
+   "namespace qi {\n"
+   "namespace unicode { namespace foo { class bar; } }\n"
+   "}\n"
+   "}\n"
+   "}",
+   style);
+
+  style.ColumnLimit = 0;
+  verifyFormat("namespace boost { namespace spirit { namespace qi { namespace "
+   "unicode { namespace foo { class bar; } } } } }",
+   style);
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: lib/Format/UnwrappedLineFormatter.cpp
===
--- lib/Format/UnwrappedLineFormatter.cpp
+++ lib/Format/UnwrappedLineFormatter.cpp
@@ -199,6 +199,11 @@
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
 if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  re