[PATCH] D37904: [clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.
krasimir accepted this revision. krasimir added a comment. This revision is now accepted and ready to land. Looks good! I must have gotten confused with my test suggestion. Do you have commit access, or should I commit this for you? https://reviews.llvm.org/D37904 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D37904: [clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.
I'm currently at cppcon with limited bandwidth for reviews. I believe that the literal test that I mentioned put inside the namespace fixer test suite fails before your changes, could you please double check? On Tue, 26 Sep 2017, 15:30 Marek Kurdej via Phabricator, < revi...@reviews.llvm.org> wrote: > curdeius added a comment. > > Any thoughts on this? > > > https://reviews.llvm.org/D37904 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37904: [clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.
curdeius added a comment. Any thoughts on this? https://reviews.llvm.org/D37904 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37904: [clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.
curdeius added a comment. That's precisely what I've written, but, as I'd said before, such tests pass already without any modification in `NamespaceEndCommentsFixer`. https://reviews.llvm.org/D37904 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37904: [clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.
krasimir added a comment.
This is how you could add a test in `NamespaceEndCommentsFixerTest.cpp`:
TEST_F(NamespaceEndCommentsFixerTest, FixesNamespaceCommentsInAllmanStyle) {
FormatStyle AllmanStyle = getLLVMStyle();
AllmanStyle.BreakBeforeBraces = FormatStyle::BS_Allman;
EXPECT_EQ("namespace a\n"
"{\n"
"void f();\n"
"void g();\n"
"}// namespace a\n",
fixNamespaceEndComments("namespace a\n"
"{\n"
"void f();\n"
"void g();\n"
"}\n",
AllmanStyle));
}
https://reviews.llvm.org/D37904
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37904: [clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.
curdeius added a comment.
I confirm what I observed before: when invoking tests in
`unittests/Format/NamespaceEndCommentsFixerTest.cpp`, the `const AnnotatedLine
*line` parameter in `getNamespaceToken` gets one big line that includes both
`namespace` and `{` (something like `namespace\n{\n...`, whereas in tests
invoked from `unittests/Format/FormatTests.cpp`, there is a separate line with
`namespace\n` and another one with `{\n`.
I haven't looked at what provokes this behavior, but I imagine that there are
additional passes in `FormatTests`.
https://reviews.llvm.org/D37904
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37904: [clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.
curdeius added a comment. In https://reviews.llvm.org/D37904#873860, @krasimir wrote: > Could you please move the test that adds a namespace end comment to > `unittests/Format/NamespaceEndCommentsFixerTest.cpp`? That's what I wanted to do initially, but the very same tests there passed surprisingly. I think that there are some differences related to "messing up" the code between both test suites. I'll have another look at it however. https://reviews.llvm.org/D37904 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37904: [clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.
krasimir added a comment. Could you please move the test that adds a namespace end comment to `unittests/Format/NamespaceEndCommentsFixerTest.cpp`? https://reviews.llvm.org/D37904 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37904: [clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.
curdeius created this revision.
Herald added a subscriber: klimek.
NamespaceEndCommentsFixer did not fix namespace comments when the brace opening
the namespace was not on the same line as the "namespace" keyword.
It occurs in Allman, GNU and Linux styles and whenever
BraceWrapping.AfterNamespace is true.
Before:
namespace a
{
void f();
void g();
}
After:
namespace a
{
void f();
void g();
} // namespace a
https://reviews.llvm.org/D37904
Files:
lib/Format/NamespaceEndCommentsFixer.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1413,7 +1413,7 @@
verifyFormat("typedef enum {} EmptyEnum;");
verifyFormat("typedef enum { A, B, C } ShortEnum;");
verifyFormat("typedef enum {\n"
- " ZERO = 0,\n"
+ " ZERO = 0,\n"
" ONE = 1,\n"
" TWO = 2,\n"
" THREE = 3\n"
@@ -1425,7 +1425,7 @@
verifyFormat("typedef enum { A, B, C } ShortEnum;");
verifyFormat("typedef enum\n"
"{\n"
- " ZERO = 0,\n"
+ " ZERO = 0,\n"
" ONE = 1,\n"
" TWO = 2,\n"
" THREE = 3\n"
@@ -9323,7 +9323,7 @@
"struct B {\n"
" int x;\n"
"};\n"
- "}\n",
+ "} // namespace a\n",
LinuxBraceStyle);
verifyFormat("enum X {\n"
" Y = 0,\n"
@@ -9453,6 +9453,19 @@
TEST_F(FormatTest, AllmanBraceBreaking) {
FormatStyle AllmanBraceStyle = getLLVMStyle();
AllmanBraceStyle.BreakBeforeBraces = FormatStyle::BS_Allman;
+
+ EXPECT_EQ("namespace a\n"
+"{\n"
+"void f();\n"
+"void g();\n"
+"} // namespace a\n",
+format("namespace a\n"
+ "{\n"
+ "void f();\n"
+ "void g();\n"
+ "}\n",
+ AllmanBraceStyle));
+
verifyFormat("namespace a\n"
"{\n"
"class A\n"
@@ -9471,7 +9484,7 @@
"{\n"
" int x;\n"
"};\n"
- "}",
+ "} // namespace a",
AllmanBraceStyle);
verifyFormat("void f()\n"
@@ -9677,7 +9690,7 @@
" }\n"
" void g() { return; }\n"
"}\n"
- "}",
+ "} // namespace a",
GNUBraceStyle);
verifyFormat("void f()\n"
Index: lib/Format/NamespaceEndCommentsFixer.cpp
===
--- lib/Format/NamespaceEndCommentsFixer.cpp
+++ lib/Format/NamespaceEndCommentsFixer.cpp
@@ -118,6 +118,12 @@
return nullptr;
assert(StartLineIndex < AnnotatedLines.size());
const FormatToken *NamespaceTok = AnnotatedLines[StartLineIndex]->First;
+ if (NamespaceTok->is(tok::l_brace)) {
+// "namespace" keyword can be on the line preceding '{', e.g. in styles
+// where BraceWrapping.AfterNamespace is true.
+if (StartLineIndex > 0)
+ NamespaceTok = AnnotatedLines[StartLineIndex - 1]->First;
+ }
// Detect "(inline)? namespace" in the beginning of a line.
if (NamespaceTok->is(tok::kw_inline))
NamespaceTok = NamespaceTok->getNextNonComment();
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -1413,7 +1413,7 @@
verifyFormat("typedef enum {} EmptyEnum;");
verifyFormat("typedef enum { A, B, C } ShortEnum;");
verifyFormat("typedef enum {\n"
- " ZERO = 0,\n"
+ " ZERO = 0,\n"
" ONE = 1,\n"
" TWO = 2,\n"
" THREE = 3\n"
@@ -1425,7 +1425,7 @@
verifyFormat("typedef enum { A, B, C } ShortEnum;");
verifyFormat("typedef enum\n"
"{\n"
- " ZERO = 0,\n"
+ " ZERO = 0,\n"
" ONE = 1,\n"
" TWO = 2,\n"
" THREE = 3\n"
@@ -9323,7 +9323,7 @@
"struct B {\n"
" int x;\n"
"};\n"
- "}\n",
+ "} // namespace a\n",
LinuxBraceStyle);
verifyFormat("enum X {\n"
" Y = 0,\n"
@@ -9453,6 +9453,19 @@
TEST_F(FormatTest, AllmanBraceBreaking) {
FormatStyle AllmanBraceStyle = getLLVMStyle();
AllmanBraceStyle.BreakBeforeBraces = FormatStyle::BS_Allman;
+
+ EXPECT_EQ("namespace a\n"
+"{\n"
+"void f();\n"
+"void g();\n"
+"} // namespace a\n",
+format("namespace a\n"
+ "{\n"
+ "void f();\n"
+ "void
