[PATCH] D32480: clang-format: Add CompactNamespaces option
This revision was automatically updated to reflect the committed changes.
Closed by commit rL305384: clang-format: Add CompactNamespaces option (authored
by Typz).
Changed prior to commit:
https://reviews.llvm.org/D32480?vs=102528=102531#toc
Repository:
rL LLVM
https://reviews.llvm.org/D32480
Files:
cfe/trunk/include/clang/Format/Format.h
cfe/trunk/lib/Format/Format.cpp
cfe/trunk/lib/Format/NamespaceEndCommentsFixer.cpp
cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
cfe/trunk/lib/Format/UnwrappedLineParser.cpp
cfe/trunk/unittests/Format/FormatTest.cpp
cfe/trunk/unittests/Format/NamespaceEndCommentsFixerTest.cpp
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -1310,6 +1310,141 @@
Style));
}
+TEST_F(FormatTest, FormatsCompactNamespaces) {
+ FormatStyle Style = getLLVMStyle();
+ Style.CompactNamespaces = true;
+
+ verifyFormat("namespace A { namespace B {\n"
+ "}} // namespace A::B",
+ Style);
+
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+ "} // namespace out",
+ Style));
+
+ // Only namespaces which have both consecutive opening and end get compacted
+ EXPECT_EQ("namespace out {\n"
+"namespace in1 {\n"
+"} // namespace in1\n"
+"namespace in2 {\n"
+"} // namespace in2\n"
+"} // namespace out",
+format("namespace out {\n"
+ "namespace in1 {\n"
+ "} // namespace in1\n"
+ "namespace in2 {\n"
+ "} // namespace in2\n"
+ "} // namespace out",
+ Style));
+
+ EXPECT_EQ("namespace out {\n"
+"int i;\n"
+"namespace in {\n"
+"int j;\n"
+"} // namespace in\n"
+"int k;\n"
+"} // namespace out",
+format("namespace out { int i;\n"
+ "namespace in { int j; } // namespace in\n"
+ "int k; } // namespace out",
+ Style));
+
+ EXPECT_EQ("namespace A { namespace B { namespace C {\n"
+"}}} // namespace A::B::C\n",
+format("namespace A { namespace B {\n"
+ "namespace C {\n"
+ "}} // namespace B::C\n"
+ "} // namespace A\n",
+ Style));
+
+ Style.ColumnLimit = 40;
+ EXPECT_EQ("namespace aa {\n"
+"namespace bb {\n"
+"}} // namespace aa::bb",
+format("namespace aa {\n"
+ "namespace bb {\n"
+ "} // namespace bb\n"
+ "} // namespace aa",
+ Style));
+
+ EXPECT_EQ("namespace aa { namespace bb {\n"
+"namespace cc {\n"
+"}}} // namespace aa::bb::cc",
+format("namespace aa {\n"
+ "namespace bb {\n"
+ "namespace cc {\n"
+ "} // namespace cc\n"
+ "} // namespace bb\n"
+ "} // namespace aa",
+ Style));
+ Style.ColumnLimit = 80;
+
+ // Extra semicolon after 'inner' closing brace prevents merging
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}; } // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "}; // namespace in\n"
+ "} // namespace out",
+ Style));
+
+ // Extra semicolon after 'outer' closing brace is conserved
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}}; // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+ "}; // namespace out",
+ Style));
+
+ Style.NamespaceIndentation = FormatStyle::NI_All;
+ EXPECT_EQ("namespace out { namespace in {\n"
+" int i;\n"
+"}} // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "int i;\n"
+ "} // namespace in\n"
+ "} // namespace out",
+ Style));
+ EXPECT_EQ("namespace out { namespace mid {\n"
+" namespace in {\n"
+"int j;\n"
+" } // namespace in\n"
+" int k;\n"
+"}} // namespace out::mid",
+format("namespace out { namespace mid {\n"
+ "namespace in { int j; } // namespace in\n"
+ "int k; }}
[PATCH] D32480: clang-format: Add CompactNamespaces option
Typz updated this revision to Diff 102528.
Typz marked an inline comment as done.
Typz added a comment.
Move tests that add or fix namespace end comments to
NamespaceEndCommentsFixerTest.cpp
https://reviews.llvm.org/D32480
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Format/NamespaceEndCommentsFixer.cpp
lib/Format/UnwrappedLineFormatter.cpp
lib/Format/UnwrappedLineParser.cpp
unittests/Format/FormatTest.cpp
unittests/Format/NamespaceEndCommentsFixerTest.cpp
Index: unittests/Format/NamespaceEndCommentsFixerTest.cpp
===
--- unittests/Format/NamespaceEndCommentsFixerTest.cpp
+++ unittests/Format/NamespaceEndCommentsFixerTest.cpp
@@ -185,6 +185,41 @@
"}\n"
"}"));
+ // Add comment for namespaces which will be 'compacted'
+ FormatStyle CompactNamespacesStyle = getLLVMStyle();
+ CompactNamespacesStyle.CompactNamespaces = true;
+ EXPECT_EQ("namespace out { namespace in {\n"
+"int i;\n"
+"int j;\n"
+"}}// namespace out::in",
+fixNamespaceEndComments("namespace out { namespace in {\n"
+"int i;\n"
+"int j;\n"
+"}}",
+CompactNamespacesStyle));
+ EXPECT_EQ("namespace out {\n"
+"namespace in {\n"
+"int i;\n"
+"int j;\n"
+"}\n"
+"}// namespace out::in",
+fixNamespaceEndComments("namespace out {\n"
+"namespace in {\n"
+"int i;\n"
+"int j;\n"
+"}\n"
+"}",
+CompactNamespacesStyle));
+ EXPECT_EQ("namespace out { namespace in {\n"
+"int i;\n"
+"int j;\n"
+"};}// namespace out::in",
+fixNamespaceEndComments("namespace out { namespace in {\n"
+"int i;\n"
+"int j;\n"
+"};}",
+CompactNamespacesStyle));
+
// Adds an end comment after a semicolon.
EXPECT_EQ("namespace {\n"
" int i;\n"
@@ -388,6 +423,27 @@
fixNamespaceEndComments("namespace A {} // namespace"));
EXPECT_EQ("namespace A {}; // namespace A",
fixNamespaceEndComments("namespace A {}; // namespace"));
+
+ // Update invalid comments for compacted namespaces.
+ FormatStyle CompactNamespacesStyle = getLLVMStyle();
+ CompactNamespacesStyle.CompactNamespaces = true;
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+fixNamespaceEndComments("namespace out { namespace in {\n"
+"}} // namespace out",
+CompactNamespacesStyle));
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+fixNamespaceEndComments("namespace out { namespace in {\n"
+"}} // namespace in",
+CompactNamespacesStyle));
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}\n"
+"} // namespace out::in",
+fixNamespaceEndComments("namespace out { namespace in {\n"
+"}// banamespace in\n"
+"} // namespace out",
+CompactNamespacesStyle));
}
TEST_F(NamespaceEndCommentsFixerTest, UpdatesInvalidEndBlockComment) {
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1309,6 +1309,141 @@
Style));
}
+TEST_F(FormatTest, FormatsCompactNamespaces) {
+ FormatStyle Style = getLLVMStyle();
+ Style.CompactNamespaces = true;
+
+ verifyFormat("namespace A { namespace B {\n"
+ "}} // namespace A::B",
+ Style);
+
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+ "} // namespace out",
+ Style));
+
+ // Only namespaces which have both consecutive opening and end get compacted
+ EXPECT_EQ("namespace out {\n"
+"namespace in1 {\n"
+"} // namespace in1\n"
+"namespace in2 {\n"
+"} // namespace in2\n"
+"} // namespace out",
+format("namespace out {\n"
+ "namespace in1 {\n"
+ "} // namespace in1\n"
+
[PATCH] D32480: clang-format: Add CompactNamespaces option
krasimir added inline comments. Comment at: unittests/Format/FormatTest.cpp:1394 + "int j;\n" + "}\n" + "}", Re-indent this line. https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
krasimir added a comment. Looks good, except for the tests that actually add or fix namespace end comments should be moved to NamespaceEndCommentsFixerTest.cpp. https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
Typz updated this revision to Diff 102525.
Typz added a comment.
Make tests more readable
https://reviews.llvm.org/D32480
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Format/NamespaceEndCommentsFixer.cpp
lib/Format/UnwrappedLineFormatter.cpp
lib/Format/UnwrappedLineParser.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1309,6 +1309,166 @@
Style));
}
+TEST_F(FormatTest, FormatsCompactNamespaces) {
+ FormatStyle Style = getLLVMStyle();
+ Style.CompactNamespaces = true;
+
+ verifyFormat("namespace A { namespace B {\n"
+ "}} // namespace A::B",
+ Style);
+
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+ "} // namespace out",
+ Style));
+
+ // Only namespaces which have both consecutive opening and end get compacted
+ EXPECT_EQ("namespace out {\n"
+"namespace in1 {\n"
+"} // namespace in1\n"
+"namespace in2 {\n"
+"} // namespace in2\n"
+"} // namespace out",
+format("namespace out {\n"
+ "namespace in1 {\n"
+ "} // namespace in1\n"
+ "namespace in2 {\n"
+ "} // namespace in2\n"
+ "} // namespace out",
+ Style));
+
+ EXPECT_EQ("namespace out {\n"
+"int i;\n"
+"namespace in {\n"
+"int j;\n"
+"} // namespace in\n"
+"int k;\n"
+"} // namespace out",
+format("namespace out { int i;\n"
+ "namespace in { int j; } // namespace in\n"
+ "int k; } // namespace out",
+ Style));
+
+ EXPECT_EQ("namespace A { namespace B { namespace C {\n"
+"}}} // namespace A::B::C\n",
+format("namespace A { namespace B {\n"
+ "namespace C {\n"
+ "}} // namespace B::C\n"
+ "} // namespace A\n",
+ Style));
+
+ Style.ColumnLimit = 40;
+ EXPECT_EQ("namespace aa {\n"
+"namespace bb {\n"
+"}} // namespace aa::bb",
+format("namespace aa {\n"
+ "namespace bb {\n"
+ "} // namespace bb\n"
+ "} // namespace aa",
+ Style));
+
+ EXPECT_EQ("namespace aa { namespace bb {\n"
+"namespace cc {\n"
+"}}} // namespace aa::bb::cc",
+format("namespace aa {\n"
+ "namespace bb {\n"
+ "namespace cc {\n"
+ "} // namespace cc\n"
+ "} // namespace bb\n"
+ "} // namespace aa",
+ Style));
+ Style.ColumnLimit = 80;
+
+ // Missing comments are added
+ EXPECT_EQ("namespace out { namespace in {\n"
+ "int i;\n"
+ "int j;\n"
+"}} // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "int i;\n"
+ "int j;\n"
+ "}\n"
+ "}",
+ Style));
+
+ // Incorrect comments are fixed
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+format("namespace out { namespace in {\n"
+ "}} // namespace out",
+ Style));
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+format("namespace out { namespace in {\n"
+ "}} // namespace in",
+ Style));
+
+ // Extra semicolon after 'inner' closing brace prevents merging
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}; } // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "}; // namespace in\n"
+ "} // namespace out",
+ Style));
+
+ // Extra semicolon after 'outer' closing brace is conserved
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}}; // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+ "}; // namespace out",
+ Style));
+
+ Style.NamespaceIndentation = FormatStyle::NI_All;
+ EXPECT_EQ("namespace out { namespace in {\n"
+" int i;\n"
+"}} // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+
[PATCH] D32480: clang-format: Add CompactNamespaces option
djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Yeah, looks good. Krasimir, any further concerns? https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
klimek added a comment.
Generally LG from my side.
Comment at: unittests/Format/FormatTest.cpp:1363-1381
+ EXPECT_EQ("namespace {\n"
+ "namespace {\n"
+"}} // namespace
::",
+format("namespace {\n"
+ "namespace {\n"
+ "} // namespace \n"
+ "} // namespace ",
These tests become more readable if you set up a style with a smaller column
limit.
https://reviews.llvm.org/D32480
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
Typz updated this revision to Diff 102346.
Typz added a comment.
- make "compacted" namespaces always add at most one level of indentation
- compact only namespaces which both start and end on consecutive lines
https://reviews.llvm.org/D32480
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Format/NamespaceEndCommentsFixer.cpp
lib/Format/UnwrappedLineFormatter.cpp
lib/Format/UnwrappedLineParser.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1309,6 +1309,164 @@
Style));
}
+TEST_F(FormatTest, FormatsCompactNamespaces) {
+ FormatStyle Style = getLLVMStyle();
+ Style.CompactNamespaces = true;
+
+ verifyFormat("namespace A { namespace B {\n"
+ "}} // namespace A::B",
+ Style);
+
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+ "} // namespace out",
+ Style));
+
+ // Only namespaces which have both consecutive opening and end get compacted
+ EXPECT_EQ("namespace out {\n"
+"namespace in1 {\n"
+"} // namespace in1\n"
+"namespace in2 {\n"
+"} // namespace in2\n"
+"} // namespace out",
+format("namespace out {\n"
+ "namespace in1 {\n"
+ "} // namespace in1\n"
+ "namespace in2 {\n"
+ "} // namespace in2\n"
+ "} // namespace out",
+ Style));
+
+ EXPECT_EQ("namespace out {\n"
+"int i;\n"
+"namespace in {\n"
+"int j;\n"
+"} // namespace in\n"
+"int k;\n"
+"} // namespace out",
+format("namespace out { int i;\n"
+ "namespace in { int j; } // namespace in\n"
+ "int k; } // namespace out",
+ Style));
+
+ EXPECT_EQ("namespace A { namespace B { namespace C {\n"
+"}}} // namespace A::B::C\n",
+format("namespace A { namespace B {\n"
+ "namespace C {\n"
+ "}} // namespace B::C\n"
+ "} // namespace A\n",
+ Style));
+
+ EXPECT_EQ("namespace {\n"
+ "namespace {\n"
+"}} // namespace ::",
+format("namespace {\n"
+ "namespace {\n"
+ "} // namespace \n"
+ "} // namespace ",
+ Style));
+
+ EXPECT_EQ("namespace a { namespace b {\n"
+ "namespace c {\n"
+"}}} // namespace a::b::c",
+format("namespace a {\n"
+ "namespace b {\n"
+ "namespace c {\n"
+ "} // namespace c\n"
+ "} // namespace b\n"
+ "} // namespace a",
+ Style));
+
+ // Missing comments are added
+ EXPECT_EQ("namespace out { namespace in {\n"
+ "int i;\n"
+ "int j;\n"
+"}} // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "int i;\n"
+ "int j;\n"
+ "}\n"
+ "}",
+ Style));
+
+ // Incorrect comments are fixed
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+format("namespace out { namespace in {\n"
+ "}} // namespace out",
+ Style));
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+format("namespace out { namespace in {\n"
+ "}} // namespace in",
+ Style));
+
+ // Extra semicolon after 'inner' closing brace prevents merging
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}; } // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "}; // namespace in\n"
+ "} // namespace out",
+ Style));
+
+ // Extra semicolon after 'outer' closing brace is conserved
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}}; // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+
[PATCH] D32480: clang-format: Add CompactNamespaces option
djasper added a comment. Ok. Works for me. https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
klimek added a comment. In https://reviews.llvm.org/D32480#773807, @Typz wrote: > So how do I proceed? > > 1. Keep the CompactNamespace option, and make "compacted" namespaces always > add at most one level of indentation > 2. Or assume that this can only ever usefully work with the behavior of > NI_None and add an additional enum value NI_Compact. I'd vote for (1). > And should we limit 'compacting' to namespace which start and end at > consecutive lines [e.g. exactly the same conditions as C++17 nested > namespaces], or is the current implementation enough [merge all adjacent > beginning and all adjacent endings] ? I'd vote for the former. https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
Typz added a comment. ping? https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
Typz added a comment. So how do I proceed? 1. Keep the CompactNamespace option, and make "compacted" namespaces always add at most one level of indentation 2. Or assume that this can only ever usefully work with the behavior of NI_None and add an additional enum value NI_Compact. And should we limit 'compacting' to namespace which start and end at consecutive lines [e.g. exactly the same conditions as C++17 nested namespaces], or is the current implementation enough [merge all adjacent beginning and all adjacent endings] ? https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
Typz added a comment. That will be slightly more complicated to check, esp. we will need to keep track of the matching-closing-brace (currently only the matching-opening-brace) is stored. But I can try to update the patch in that direction, if that is the consensus. https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
klimek added a comment. I'm less concerned about everything suddenly re-indenting when you change code - if you use any kind of namespace indentation, that's what will happen now and then (and is why many style guides do not indent in namespaces). For what it's worth, I'd 1. vote for only merging in cases where both start and end can be merged 2. vote for merge namespaces counting as a single namespace decl https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
Typz added a comment. ping? https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
Typz updated this revision to Diff 99763.
Typz added a comment.
Remove dependency on https://reviews.llvm.org/D33314
https://reviews.llvm.org/D32480
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Format/NamespaceEndCommentsFixer.cpp
lib/Format/UnwrappedLineFormatter.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1309,6 +1309,140 @@
Style));
}
+TEST_F(FormatTest, FormatsCompactNamespaces) {
+ FormatStyle Style = getLLVMStyle();
+ Style.CompactNamespaces = true;
+
+ verifyFormat("namespace A { namespace B {\n"
+ "}} // namespace A::B",
+ Style);
+
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+ "} // namespace out",
+ Style));
+
+ EXPECT_EQ("namespace out { namespace in1 {\n"
+"} // namespace in1\n"
+"namespace in2 {\n"
+"}} // namespace out::in2",
+format("namespace out {\n"
+ "namespace in1 {\n"
+ "} // namespace in1\n"
+ "namespace in2 {\n"
+ "} // namespace in2\n"
+ "} // namespace out",
+ Style));
+
+ EXPECT_EQ("namespace out {\n"
+"int i;\n"
+"namespace in {\n"
+"int j;\n"
+"} // namespace in\n"
+"int k;\n"
+"} // namespace out",
+format("namespace out { int i;\n"
+ "namespace in { int j; } // namespace in\n"
+ "int k; } // namespace out",
+ Style));
+
+ EXPECT_EQ("namespace A { namespace B { namespace C {\n"
+"}}} // namespace A::B::C\n",
+format("namespace A { namespace B {\n"
+ "namespace C {\n"
+ "}} // namespace B::C\n"
+ "} // namespace A\n",
+ Style));
+
+ EXPECT_EQ("namespace {\n"
+ "namespace {\n"
+"}} // namespace ::",
+format("namespace {\n"
+ "namespace {\n"
+ "} // namespace \n"
+ "} // namespace ",
+ Style));
+
+ EXPECT_EQ("namespace a { namespace b {\n"
+ "namespace c {\n"
+"}}} // namespace a::b::c",
+format("namespace a {\n"
+ "namespace b {\n"
+ "namespace c {\n"
+ "} // namespace c\n"
+ "} // namespace b\n"
+ "} // namespace a",
+ Style));
+
+ // Missing comments are added
+ EXPECT_EQ("namespace out { namespace in {\n"
+ "int i;\n"
+ "int j;\n"
+"}} // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "int i;\n"
+ "int j;\n"
+ "}\n"
+ "}",
+ Style));
+
+ // Incorrect comments are fixed
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+format("namespace out { namespace in {\n"
+ "}} // namespace out",
+ Style));
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+format("namespace out { namespace in {\n"
+ "}} // namespace in",
+ Style));
+
+ // Extra semicolon after 'inner' closing brace prevents merging
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}; } // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "}; // namespace in\n"
+ "} // namespace out",
+ Style));
+
+ // Extra semicolon after 'outer' closing brace is conserved
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}}; // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+ "}; // namespace out",
+ Style));
+
+ Style.NamespaceIndentation = FormatStyle::NI_All;
+ EXPECT_EQ("namespace out { namespace in {\n"
+"int i;\n"
+"}} // namespace out::in",
+
[PATCH] D32480: clang-format: Add CompactNamespaces option
Typz added a comment. Merging the 2 options is definitely a "safe" option, as it prevents ensures only the most obvious behavior is accessible. However, it has significant (IMO) drawbacks: - "Compact" is a not an namespace indentation type, this will make the option quite confusing - If indentation inside compact namespaces is needed, it cannot easily be added: we would need an extra mode NI_CompactWithIndent All in all, I think I prefer the current behavior of the patch (a separate CompactNamespace options, with consistent [if not useful] indentation settings); but it is your call, just let me know how to proceed. https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
djasper added a comment. What I mean is that you should remove the CompactNamespace option and instead let this be configured by an additional enum value in NamespaceIndentation. https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
Typz added inline comments.
Comment at: include/clang/Format/Format.h:358
+ /// \endcode
+ bool BinPackNamespaces;
+
djasper wrote:
> Typz wrote:
> > djasper wrote:
> > > Typz wrote:
> > > > djasper wrote:
> > > > > This is not bin packing at all. Maybe CompactNamespaces? Or
> > > > > SingleLineNamespaces?
> > > > >
> > > > > Also, could you clarify what happens if the namespaces don't fit
> > > > > within the column limit (both in the comment describing the flag and
> > > > > by adding tests for it)?
> > > > This is not binpacking, but has a similar effect and made the option
> > > > name somewhat consistent with the other binpack options :-)
> > > > I'll change to CompactNamespaces then.
> > > How does this option interact with NamespaceIndentation. Do all the
> > > values you can set there make sense and work as expected?
> > >
> > > I am wondering whether we should generally rename this to NamespaceStyle
> > > and make it an enum. That way, we can start to also support C++17
> > > namespace, but people that don't want to use C++17 yet, can still use
> > > this style of compact namespace.
> > > How does this option interact with NamespaceIndentation. Do all the
> > > values you can set there make sense and work as expected?
> >
> > NamespaceIndentation is not affected, indent is done as before (e.g. just
> > "counting" the imbricated namespaces).
> >
> > In 'NI_All' we may want to reduce the indent when multiple namespaces are
> > declared in the same line, but this would become inconsistent if the
> > beginning and end of all namespaces do not match:
> >
> > namepace A { namespace B {
> > int i;
> > } // namespace B
> > int i;
> > } // namespace A
> >
> > So I think reducing indent in that case should be (if ever needed) another
> > value in NamespaceIndentation...
> >
> > > I am wondering whether we should generally rename this to NamespaceStyle
> > > and make it an enum. That way, we can start to also support C++17
> > > namespace, but people that don't want to use C++17 yet, can still use
> > > this style of compact namespace.
> >
> > As for C++17, I am not sure we need another option: just having
> > CompactNamespaces=true and Standard=C++17 would use the "real" C++17 mode.
> > That said converting to C++17 namespace blocks is slightly more
> > restrictive, as it will require that both the beginning and end of the
> > inner & outer blocks to match...
> >
> > I will keep the boolean flag for now, just let me know if you prefer to
> > have the enum in case other modes are needed and I will update the patch.
> Yeah, this is probably something nobody will ever want:
>
> namepace A { namespace B {
> int i;
> } // namespace B
> int i;
> } // namespace A
>
> And you have the same problem for NI_Inner as soon as you have more than two
> levels of namespace.
>
> I see two ways forward:
>
> 1. Make "compacted" namespaces always add at most one level of indentation.
> 2. Assume that this can only ever usefully work with the behavior of NI_None
> and add an additional enum value NI_Compact.
>
> The problem I have with #1 is that it increases the complexity quite a bit
> and the behavior is even difficult to predict to users. Remove a comment
> somewhere might enable clang-format to make two namespaces "compact" and then
> remove indentation throughout the file.
>
> So I would lean more towards solution #2. The name NamespaceIndentation might
> then be a bit confusing, but not sure whether it's worth changing it. And of
> course I don't know whether some people would want indentation in compacted
> namespaces.
>
> What do you think?
I think current behavior is at least consistent (and simple to implement), and
I agree that #2 would be the way to go to implement special nanespace indent
rules for compacted namespaces.
Personnally, I don't need namespace indentation (we use NI_None), but i can add
try to add an extra style NI_Compact to indent one level when in any number of
namespaces (e.g. indentLevel = namespaceDepth>0).
That should probably be a separate patch however?
https://reviews.llvm.org/D32480
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
djasper added inline comments.
Comment at: include/clang/Format/Format.h:358
+ /// \endcode
+ bool BinPackNamespaces;
+
Typz wrote:
> djasper wrote:
> > Typz wrote:
> > > djasper wrote:
> > > > This is not bin packing at all. Maybe CompactNamespaces? Or
> > > > SingleLineNamespaces?
> > > >
> > > > Also, could you clarify what happens if the namespaces don't fit within
> > > > the column limit (both in the comment describing the flag and by adding
> > > > tests for it)?
> > > This is not binpacking, but has a similar effect and made the option name
> > > somewhat consistent with the other binpack options :-)
> > > I'll change to CompactNamespaces then.
> > How does this option interact with NamespaceIndentation. Do all the values
> > you can set there make sense and work as expected?
> >
> > I am wondering whether we should generally rename this to NamespaceStyle
> > and make it an enum. That way, we can start to also support C++17
> > namespace, but people that don't want to use C++17 yet, can still use this
> > style of compact namespace.
> > How does this option interact with NamespaceIndentation. Do all the values
> > you can set there make sense and work as expected?
>
> NamespaceIndentation is not affected, indent is done as before (e.g. just
> "counting" the imbricated namespaces).
>
> In 'NI_All' we may want to reduce the indent when multiple namespaces are
> declared in the same line, but this would become inconsistent if the
> beginning and end of all namespaces do not match:
>
> namepace A { namespace B {
> int i;
> } // namespace B
> int i;
> } // namespace A
>
> So I think reducing indent in that case should be (if ever needed) another
> value in NamespaceIndentation...
>
> > I am wondering whether we should generally rename this to NamespaceStyle
> > and make it an enum. That way, we can start to also support C++17
> > namespace, but people that don't want to use C++17 yet, can still use this
> > style of compact namespace.
>
> As for C++17, I am not sure we need another option: just having
> CompactNamespaces=true and Standard=C++17 would use the "real" C++17 mode.
> That said converting to C++17 namespace blocks is slightly more restrictive,
> as it will require that both the beginning and end of the inner & outer
> blocks to match...
>
> I will keep the boolean flag for now, just let me know if you prefer to have
> the enum in case other modes are needed and I will update the patch.
Yeah, this is probably something nobody will ever want:
namepace A { namespace B {
int i;
} // namespace B
int i;
} // namespace A
And you have the same problem for NI_Inner as soon as you have more than two
levels of namespace.
I see two ways forward:
1. Make "compacted" namespaces always add at most one level of indentation.
2. Assume that this can only ever usefully work with the behavior of NI_None
and add an additional enum value NI_Compact.
The problem I have with #1 is that it increases the complexity quite a bit and
the behavior is even difficult to predict to users. Remove a comment somewhere
might enable clang-format to make two namespaces "compact" and then remove
indentation throughout the file.
So I would lean more towards solution #2. The name NamespaceIndentation might
then be a bit confusing, but not sure whether it's worth changing it. And of
course I don't know whether some people would want indentation in compacted
namespaces.
What do you think?
https://reviews.llvm.org/D32480
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
Typz added inline comments.
Comment at: include/clang/Format/Format.h:358
+ /// \endcode
+ bool BinPackNamespaces;
+
djasper wrote:
> Typz wrote:
> > djasper wrote:
> > > This is not bin packing at all. Maybe CompactNamespaces? Or
> > > SingleLineNamespaces?
> > >
> > > Also, could you clarify what happens if the namespaces don't fit within
> > > the column limit (both in the comment describing the flag and by adding
> > > tests for it)?
> > This is not binpacking, but has a similar effect and made the option name
> > somewhat consistent with the other binpack options :-)
> > I'll change to CompactNamespaces then.
> How does this option interact with NamespaceIndentation. Do all the values
> you can set there make sense and work as expected?
>
> I am wondering whether we should generally rename this to NamespaceStyle and
> make it an enum. That way, we can start to also support C++17 namespace, but
> people that don't want to use C++17 yet, can still use this style of compact
> namespace.
> How does this option interact with NamespaceIndentation. Do all the values
> you can set there make sense and work as expected?
NamespaceIndentation is not affected, indent is done as before (e.g. just
"counting" the imbricated namespaces).
In 'NI_All' we may want to reduce the indent when multiple namespaces are
declared in the same line, but this would become inconsistent if the beginning
and end of all namespaces do not match:
namepace A { namespace B {
int i;
} // namespace B
int i;
} // namespace A
So I think reducing indent in that case should be (if ever needed) another
value in NamespaceIndentation...
> I am wondering whether we should generally rename this to NamespaceStyle and
> make it an enum. That way, we can start to also support C++17 namespace, but
> people that don't want to use C++17 yet, can still use this style of compact
> namespace.
As for C++17, I am not sure we need another option: just having
CompactNamespaces=true and Standard=C++17 would use the "real" C++17 mode. That
said converting to C++17 namespace blocks is slightly more restrictive, as it
will require that both the beginning and end of the inner & outer blocks to
match...
I will keep the boolean flag for now, just let me know if you prefer to have
the enum in case other modes are needed and I will update the patch.
Comment at: lib/Format/UnwrappedLineFormatter.cpp:139
+
+bool isEndOfNamespace(const AnnotatedLine *line,
+ const SmallVectorImpl ) {
djasper wrote:
> s/line/Line/
these methods (isEndOfNamespace, ...) are somewhat duplicated in
UnwrapperLinesFormatter and NamespaceEndComments (not really, but very similar)
: should I factorize these into helper methods of FormatToken?
https://reviews.llvm.org/D32480
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
Typz updated this revision to Diff 99575.
Typz marked 11 inline comments as done.
Typz edited the summary of this revision.
Typz added a comment.
update for review comments
https://reviews.llvm.org/D32480
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Format/NamespaceEndCommentsFixer.cpp
lib/Format/UnwrappedLineFormatter.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1308,6 +1308,140 @@
Style));
}
+TEST_F(FormatTest, FormatsCompactNamespaces) {
+ FormatStyle Style = getLLVMStyle();
+ Style.CompactNamespaces = true;
+
+ verifyFormat("namespace A { namespace B {\n"
+ "}} // namespace A::B",
+ Style);
+
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+ "} // namespace out",
+ Style));
+
+ EXPECT_EQ("namespace out { namespace in1 {\n"
+"} // namespace in1\n"
+"namespace in2 {\n"
+"}} // namespace out::in2",
+format("namespace out {\n"
+ "namespace in1 {\n"
+ "} // namespace in1\n"
+ "namespace in2 {\n"
+ "} // namespace in2\n"
+ "} // namespace out",
+ Style));
+
+ EXPECT_EQ("namespace out {\n"
+"int i;\n"
+"namespace in {\n"
+"int j;\n"
+"} // namespace in\n"
+"int k;\n"
+"} // namespace out",
+format("namespace out { int i;\n"
+ "namespace in { int j; } // namespace in\n"
+ "int k; } // namespace out",
+ Style));
+
+ EXPECT_EQ("namespace A { namespace B { namespace C {\n"
+"}}} // namespace A::B::C\n",
+format("namespace A { namespace B {\n"
+ "namespace C {\n"
+ "}} // namespace B::C\n"
+ "} // namespace A\n",
+ Style));
+
+ EXPECT_EQ("namespace {\n"
+ "namespace {\n"
+"}} // namespace ::",
+format("namespace {\n"
+ "namespace {\n"
+ "} // namespace \n"
+ "} // namespace ",
+ Style));
+
+ EXPECT_EQ("namespace a { namespace b {\n"
+ "namespace c {\n"
+"}}} // namespace a::b::c",
+format("namespace a {\n"
+ "namespace b {\n"
+ "namespace c {\n"
+ "} // namespace c\n"
+ "} // namespace b\n"
+ "} // namespace a",
+ Style));
+
+ // Missing comments are added
+ EXPECT_EQ("namespace out { namespace in {\n"
+ "int i;\n"
+ "int j;\n"
+"}} // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "int i;\n"
+ "int j;\n"
+ "}\n"
+ "}",
+ Style));
+
+ // Incorrect comments are fixed
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+format("namespace out { namespace in {\n"
+ "}} // namespace out",
+ Style));
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}} // namespace out::in",
+format("namespace out { namespace in {\n"
+ "}} // namespace in",
+ Style));
+
+ // Extra semicolon after 'inner' closing brace prevents merging
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}; } // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "}; // namespace in\n"
+ "} // namespace out",
+ Style));
+
+ // Extra semicolon after 'outer' closing brace is conserved
+ EXPECT_EQ("namespace out { namespace in {\n"
+"}}; // namespace out::in",
+format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+ "}; // namespace out",
+ Style));
+
+ Style.NamespaceIndentation = FormatStyle::NI_All;
+ EXPECT_EQ("namespace out { namespace in {\n"
+"int i;\n"
+
[PATCH] D32480: clang-format: Add CompactNamespaces option
djasper added inline comments.
Comment at: include/clang/Format/Format.h:358
+ /// \endcode
+ bool BinPackNamespaces;
+
Typz wrote:
> djasper wrote:
> > This is not bin packing at all. Maybe CompactNamespaces? Or
> > SingleLineNamespaces?
> >
> > Also, could you clarify what happens if the namespaces don't fit within the
> > column limit (both in the comment describing the flag and by adding tests
> > for it)?
> This is not binpacking, but has a similar effect and made the option name
> somewhat consistent with the other binpack options :-)
> I'll change to CompactNamespaces then.
How does this option interact with NamespaceIndentation. Do all the values you
can set there make sense and work as expected?
I am wondering whether we should generally rename this to NamespaceStyle and
make it an enum. That way, we can start to also support C++17 namespace, but
people that don't want to use C++17 yet, can still use this style of compact
namespace.
Comment at: include/clang/Format/Format.h:769
+ /// If it does not fit on a single line, the overflowing namespaces get
wrapped:
+ /// /// \code
+ /// namespace Foo { namespace Bar {
What happened here?
Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:124
+
+const FormatToken * getNamespaceToken(const AnnotatedLine *line,
+ const SmallVectorImpl
) {
no space after "*"..
Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:127
+if (!line->Affected || line->InPPDirective ||
!line->startsWith(tok::r_brace))
+ return NULL;
+size_t StartLineIndex = line->MatchingOpeningBlockLineIndex;
s/NULL/nullptr/
Comment at: lib/Format/NamespaceEndCommentsFixer.cpp:172
}
+if (StartLineIndex == SIZE_MAX)
+ StartLineIndex = EndLine->MatchingOpeningBlockLineIndex;
Do we need any of this if CompactNamespaces if false? Maybe we should surround
the entire thing with an
if (Style.CompactNamespaces) {
Comment at: lib/Format/UnwrappedLineFormatter.cpp:130
+bool isNamespaceToken(const FormatToken *NamespaceTok) {
+ // Detect "(inline)? namespace" in the beginning of a line.
You always seem to call this function with Line->First. Maybe just call it on
an AnnotatedLine?
Comment at: lib/Format/UnwrappedLineFormatter.cpp:134
+NamespaceTok = NamespaceTok->getNextNonComment();
+ if (!NamespaceTok || NamespaceTok->isNot(tok::kw_namespace))
+return false;
just
return NamespaceTok && NamespaceTok->is(tok::kw_namespace);
Comment at: lib/Format/UnwrappedLineFormatter.cpp:139
+
+bool isEndOfNamespace(const AnnotatedLine *line,
+ const SmallVectorImpl ) {
s/line/Line/
Comment at: lib/Format/UnwrappedLineFormatter.cpp:221
+ if (isNamespaceToken(TheLine->First)) {
+int i = 0;
+while (I + 1 + i != E && isNamespaceToken(I[i + 1]->First) &&
This looks like you wanted to write a for loop.
Comment at: lib/Format/UnwrappedLineFormatter.cpp:223
+while (I + 1 + i != E && isNamespaceToken(I[i + 1]->First) &&
+I[i + 1]->Last->TotalLength < Limit) {
+ Limit -= I[i + 1]->Last->TotalLength;
Indentation
Comment at: unittests/Format/FormatTest.cpp:1310
+
+ FormatStyle LLVMWithCompactNamespaces = getLLVMStyle();
+ LLVMWithCompactNamespaces.CompactNamespaces = true;
There need to be more tests. Specifically:
// Input is already in the desired format
namespace A { namespace B { .. }} // namespace A::B
// Input is contracted, but wrong comment
namespace A { namespace B {..}} // namespace A
// Input is partially contracted
namespace A { namespace B { namespace C {
}} // namespace B::C
} // namespace A
https://reviews.llvm.org/D32480
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32480: clang-format: Add CompactNamespaces option
Typz marked 2 inline comments as done. Typz added inline comments. Comment at: include/clang/Format/Format.h:153 + /// \endcode + bool AllowSemicolonAfterNamespace; + Typz wrote: > djasper wrote: > > While I am not entirely opposed to this feature, I think it should be a > > separate patch. > I totally agree, which is why I created 2 commits; however, since they modify > the same code (and I am new to Phabricator) I could not find a way to upload > them separately. > > Is there a way? Or should I upload one, and upload the next one once the > first one has been accepted? OK, found the answer here: https://medium.com/@kurtisnusbaum/stacked-diffs-keeping-phabricator-diffs-small-d9964f4dcfa6 Comment at: include/clang/Format/Format.h:358 + /// \endcode + bool BinPackNamespaces; + djasper wrote: > This is not bin packing at all. Maybe CompactNamespaces? Or > SingleLineNamespaces? > > Also, could you clarify what happens if the namespaces don't fit within the > column limit (both in the comment describing the flag and by adding tests for > it)? This is not binpacking, but has a similar effect and made the option name somewhat consistent with the other binpack options :-) I'll change to CompactNamespaces then. https://reviews.llvm.org/D32480 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
