Skip to content

Commit

Permalink
[clang-format] Use number of unwrapped lines for short namespace
Browse files Browse the repository at this point in the history 
Summary:
This patch makes the namespace comment fixer use the number of unwrapped lines
that a namespace spans to detect it that namespace is short, thus not needing
end comments to be added.
This is needed to ensure clang-format is idempotent. Previously, a short namespace
was detected by the original source code lines. This has the effect of requiring two
runs for this example:
```
namespace { class A; }
```
after first run:
```
namespace {
class A;
}
```
after second run:
```
namespace {
class A;
} // namespace
```

Reviewers: djasper

Reviewed By: djasper

Subscribers: cfe-commits, klimek

Differential Revision: https://reviews.llvm.org/D30528

llvm-svn: 296736
  • Loading branch information
@krasimirgg
krasimirgg committed Mar 2, 2017
1 parent fb0dc62 commit 9163fe2
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 24 deletions.
13 changes: 3 additions & 10 deletions clang/lib/Format/NamespaceEndCommentsFixer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ namespace clang {
namespace format {

namespace {
// The maximal number of lines that a short namespace spans.
// The maximal number of unwrapped lines that a short namespace spans.
// Short namespaces don't need an end comment.
static const int kShortNamespaceMaxLines = 1;

Expand Down Expand Up @@ -60,14 +60,6 @@ std::string computeEndCommentText(StringRef NamespaceName, bool AddNewline) {
return text;
}

bool isShort(const FormatToken *NamespaceTok, const FormatToken *RBraceTok,
const SourceManager &SourceMgr) {
int StartLine =
SourceMgr.getSpellingLineNumber(NamespaceTok->Tok.getLocation());
int EndLine = SourceMgr.getSpellingLineNumber(RBraceTok->Tok.getLocation());
return EndLine - StartLine + 1 <= kShortNamespaceMaxLines;
}

bool hasEndComment(const FormatToken *RBraceTok) {
return RBraceTok->Next && RBraceTok->Next->is(tok::comment);
}
Expand Down Expand Up @@ -151,7 +143,8 @@ tooling::Replacements NamespaceEndCommentsFixer::analyze(
const std::string EndCommentText =
computeEndCommentText(NamespaceName, AddNewline);
if (!hasEndComment(RBraceTok)) {
if (!isShort(NamespaceTok, RBraceTok, SourceMgr))
bool isShort = I - StartLineIndex <= kShortNamespaceMaxLines + 1;
if (!isShort)
addEndComment(RBraceTok, EndCommentText, SourceMgr, &Fixes);
continue;
}
Expand Down
13 changes: 6 additions & 7 deletions clang/unittests/Format/FormatTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ TEST_F(FormatTest, RemovesEmptyLines) {
EXPECT_EQ("namespace N {\n"
"\n"
"int i;\n"
"} // namespace N",
"}",
format("namespace N {\n"
"\n"
"int i;\n"
Expand Down Expand Up @@ -281,8 +281,7 @@ TEST_F(FormatTest, RemovesEmptyLines) {
"}", LLVMWithNoNamespaceFix));
EXPECT_EQ("namespace {\n"
"int i;\n"
"\n"
"} // namespace",
"}",
format("namespace {\n"
"int i;\n"
"\n"
Expand Down Expand Up @@ -5460,7 +5459,7 @@ TEST_F(FormatTest, IncorrectCodeMissingSemicolon) {
EXPECT_EQ("namespace N {\n"
"void f() {}\n"
"void g()\n"
"}",
"} // namespace N",
format("namespace N { void f( ) { } void g( ) }"));
}

Expand Down Expand Up @@ -6140,8 +6139,8 @@ TEST_F(FormatTest, FormatStarDependingOnContext) {
" void f() {}\n"
" int *a;\n"
"};\n"
"}\n"
"}");
"} // namespace b\n"
"} // namespace a");
}

TEST_F(FormatTest, SpecialTokensAtEndOfLine) {
Expand Down Expand Up @@ -7934,7 +7933,7 @@ TEST_F(FormatTest, StroustrupBraceBreaking) {
"struct B {\n"
" int x;\n"
"};\n"
"}\n",
"} // namespace a\n",
StroustrupBraceStyle);

verifyFormat("void foo()\n"
Expand Down
58 changes: 51 additions & 7 deletions clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,87 +47,123 @@ class NamespaceEndCommentsFixerTest : public ::testing::Test {
TEST_F(NamespaceEndCommentsFixerTest, AddsEndComment) {
EXPECT_EQ("namespace {\n"
" int i;\n"
" int j;\n"
"}// namespace",
fixNamespaceEndComments("namespace {\n"
" int i;\n"
" int j;\n"
"}"));
EXPECT_EQ("namespace {\n"
" int i;\n"
" int j;\n"
"}// namespace\n",
fixNamespaceEndComments("namespace {\n"
" int i;\n"
" int j;\n"
"}\n"));
EXPECT_EQ("namespace A {\n"
" int i;\n"
" int j;\n"
"}// namespace A",
fixNamespaceEndComments("namespace A {\n"
" int i;\n"
" int j;\n"
"}"));
EXPECT_EQ("inline namespace A {\n"
" int i;\n"
" int j;\n"
"}// namespace A",
fixNamespaceEndComments("inline namespace A {\n"
" int i;\n"
" int j;\n"
"}"));
EXPECT_EQ("namespace ::A {\n"
" int i;\n"
" int j;\n"
"}// namespace ::A",
fixNamespaceEndComments("namespace ::A {\n"
" int i;\n"
" int j;\n"
"}"));
EXPECT_EQ("namespace ::A::B {\n"
" int i;\n"
" int j;\n"
"}// namespace ::A::B",
fixNamespaceEndComments("namespace ::A::B {\n"
" int i;\n"
" int j;\n"
"}"));
EXPECT_EQ("namespace /**/::/**/A/**/::/**/B/**/ {\n"
" int i;\n"
" int j;\n"
"}// namespace ::A::B",
fixNamespaceEndComments("namespace /**/::/**/A/**/::/**/B/**/ {\n"
" int i;\n"
" int j;\n"
"}"));
EXPECT_EQ("namespace A {\n"
"namespace B {\n"
" int i;\n"
"}\n"
"}// namespace A",
fixNamespaceEndComments("namespace A {\n"
"namespace B {\n"
" int i;\n"
"}\n"
"}"));
EXPECT_EQ("namespace A {\n"
"namespace B {\n"
" int i;\n"
" int j;\n"
"}// namespace B\n"
"}// namespace A",
fixNamespaceEndComments("namespace A {\n"
"namespace B {\n"
" int i;\n"
" int j;\n"
"}\n"
"}"));
EXPECT_EQ("namespace A {\n"
" int a;\n"
" int b;\n"
"}// namespace A\n"
"namespace B {\n"
" int b;\n"
" int a;\n"
"}// namespace B",
fixNamespaceEndComments("namespace A {\n"
" int a;\n"
" int b;\n"
"}\n"
"namespace B {\n"
" int b;\n"
" int a;\n"
"}"));
EXPECT_EQ("namespace A {\n"
" int a1;\n"
" int a2;\n"
"}// namespace A\n"
"namespace A {\n"
" int a2;\n"
" int a1;\n"
"}// namespace A",
fixNamespaceEndComments("namespace A {\n"
" int a1;\n"
" int a2;\n"
"}\n"
"namespace A {\n"
" int a2;\n"
" int a1;\n"
"}"));
EXPECT_EQ("namespace A {\n"
" int a;\n"
" int b;\n"
"}// namespace A\n"
"// comment about b\n"
"int b;",
fixNamespaceEndComments("namespace A {\n"
" int a;\n"
" int b;\n"
"}\n"
"// comment about b\n"
"int b;"));
Expand All @@ -136,7 +172,7 @@ TEST_F(NamespaceEndCommentsFixerTest, AddsEndComment) {
"namespace B {\n"
"namespace C {\n"
"namespace D {\n"
"}// namespace D\n"
"}\n"
"}// namespace C\n"
"}// namespace B\n"
"}// namespace A",
Expand All @@ -153,36 +189,44 @@ TEST_F(NamespaceEndCommentsFixerTest, AddsEndComment) {
TEST_F(NamespaceEndCommentsFixerTest, AddsNewlineIfNeeded) {
EXPECT_EQ("namespace A {\n"
" int i;\n"
" int j;\n"
"}// namespace A\n"
" int j;",
" int k;",
fixNamespaceEndComments("namespace A {\n"
" int i;\n"
"} int j;"));
" int j;\n"
"} int k;"));
EXPECT_EQ("namespace {\n"
" int i;\n"
" int j;\n"
"}// namespace\n"
" int j;",
" int k;",
fixNamespaceEndComments("namespace {\n"
" int i;\n"
"} int j;"));
" int j;\n"
"} int k;"));
EXPECT_EQ("namespace A {\n"
" int i;\n"
" int j;\n"
"}// namespace A\n"
" namespace B {\n"
" int j;\n"
" int k;\n"
"}// namespace B",
fixNamespaceEndComments("namespace A {\n"
" int i;\n"
" int j;\n"
"} namespace B {\n"
" int j;\n"
" int k;\n"
"}"));
}

TEST_F(NamespaceEndCommentsFixerTest, DoesNotAddEndCommentForShortNamespace) {
EXPECT_EQ("namespace {}", fixNamespaceEndComments("namespace {}"));
EXPECT_EQ("namespace A {}", fixNamespaceEndComments("namespace A {}"));
EXPECT_EQ("namespace A { int i; }",
fixNamespaceEndComments("namespace A { int i; }"));
EXPECT_EQ("namespace A { a }",
fixNamespaceEndComments("namespace A { a }"));
}

TEST_F(NamespaceEndCommentsFixerTest, DoesNotAddCommentAfterUnaffectedRBrace) {
Expand Down

0 comments on commit 9163fe2

Please sign in to comment.