Skip to content

Commit fac4e3c

Browse files
alexfhalexfh
committed
[clang-tidy] Fix PR26274
Summary: This commit fixes http://llvm.org/PR26274 in a simpler and more correct way than 4736d63 did. See https://reviews.llvm.org/D69855#1767089 for details. Reviewers: gribozavr, aaron.ballman, gribozavr2 Reviewed By: aaron.ballman, gribozavr2 Subscribers: gribozavr2, merge_guards_bot, xazax.hun, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D70974
1 parent 64df0f3 commit fac4e3c

File tree

3 files changed

+137
-70
lines changed

3 files changed

+137
-70
lines changed

‎clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp‎

Lines changed: 70 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,11 @@
77
//===----------------------------------------------------------------------===//
88

99
#include "NamespaceCommentCheck.h"
10+
#include "../utils/LexerUtils.h"
1011
#include "clang/AST/ASTContext.h"
1112
#include "clang/ASTMatchers/ASTMatchers.h"
1213
#include "clang/Basic/SourceLocation.h"
14+
#include "clang/Basic/TokenKinds.h"
1315
#include "clang/Lex/Lexer.h"
1416
#include "llvm/ADT/StringExtras.h"
1517

@@ -46,30 +48,48 @@ static bool locationsInSameFile(const SourceManager &Sources,
4648
Sources.getFileID(Loc1) == Sources.getFileID(Loc2);
4749
}
4850

49-
static std::string getNamespaceComment(const NamespaceDecl *ND,
50-
bool InsertLineBreak) {
51-
std::string Fix = "// namespace";
52-
if (!ND->isAnonymousNamespace())
53-
Fix.append(" ").append(ND->getNameAsString());
54-
if (InsertLineBreak)
55-
Fix.append("\n");
56-
return Fix;
57-
}
58-
59-
static std::string getNamespaceComment(const std::string &NameSpaceName,
60-
bool InsertLineBreak) {
61-
std::string Fix = "// namespace ";
62-
Fix.append(NameSpaceName);
63-
if (InsertLineBreak)
64-
Fix.append("\n");
65-
return Fix;
51+
static llvm::Optional<std::string>
52+
getNamespaceNameAsWritten(SourceLocation &Loc, const SourceManager &Sources,
53+
const LangOptions &LangOpts) {
54+
// Loc should be at the begin of the namespace decl (usually, `namespace`
55+
// token). We skip the first token right away, but in case of `inline
56+
// namespace` or `namespace a::inline b` we can see both `inline` and
57+
// `namespace` keywords, which we just ignore. Nested parens/squares before
58+
// the opening brace can result from attributes.
59+
std::string Result;
60+
int Nesting = 0;
61+
while (llvm::Optional<Token> T = utils::lexer::findNextTokenSkippingComments(
62+
Loc, Sources, LangOpts)) {
63+
Loc = T->getLocation();
64+
if (T->is(tok::l_brace))
65+
break;
66+
67+
if (T->isOneOf(tok::l_square, tok::l_paren)) {
68+
++Nesting;
69+
} else if (T->isOneOf(tok::r_square, tok::r_paren)) {
70+
--Nesting;
71+
} else if (Nesting == 0) {
72+
if (T->is(tok::raw_identifier)) {
73+
StringRef ID = T->getRawIdentifier();
74+
if (ID != "namespace" && ID != "inline")
75+
Result.append(ID);
76+
} else if (T->is(tok::coloncolon)) {
77+
Result.append("::");
78+
} else { // Any other kind of token is unexpected here.
79+
return llvm::None;
80+
}
81+
}
82+
}
83+
return Result;
6684
}
6785

6886
void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
6987
const auto *ND = Result.Nodes.getNodeAs<NamespaceDecl>("namespace");
7088
const SourceManager &Sources = *Result.SourceManager;
7189

72-
if (!locationsInSameFile(Sources, ND->getBeginLoc(), ND->getRBraceLoc()))
90+
// Ignore namespaces inside macros and namespaces split across files.
91+
if (ND->getBeginLoc().isMacroID() ||
92+
!locationsInSameFile(Sources, ND->getBeginLoc(), ND->getRBraceLoc()))
7393
return;
7494

7595
// Don't require closing comments for namespaces spanning less than certain
@@ -80,44 +100,32 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
80100
return;
81101

82102
// Find next token after the namespace closing brace.
83-
SourceLocation AfterRBrace = ND->getRBraceLoc().getLocWithOffset(1);
103+
SourceLocation AfterRBrace = Lexer::getLocForEndOfToken(
104+
ND->getRBraceLoc(), /*Offset=*/0, Sources, getLangOpts());
84105
SourceLocation Loc = AfterRBrace;
85-
Token Tok;
86-
SourceLocation LBracketLocation = ND->getLocation();
87-
SourceLocation NestedNamespaceBegin = LBracketLocation;
106+
SourceLocation LBraceLoc = ND->getBeginLoc();
88107

89108
// Currently for nested namepsace (n1::n2::...) the AST matcher will match foo
90109
// then bar instead of a single match. So if we got a nested namespace we have
91110
// to skip the next ones.
92111
for (const auto &EndOfNameLocation : Ends) {
93-
if (Sources.isBeforeInTranslationUnit(NestedNamespaceBegin,
94-
EndOfNameLocation))
112+
if (Sources.isBeforeInTranslationUnit(ND->getLocation(), EndOfNameLocation))
95113
return;
96114
}
97115

98-
// Ignore macros
99-
if (!ND->getLocation().isMacroID()) {
100-
while (Lexer::getRawToken(LBracketLocation, Tok, Sources, getLangOpts()) ||
101-
!Tok.is(tok::l_brace)) {
102-
LBracketLocation = LBracketLocation.getLocWithOffset(1);
103-
}
116+
llvm::Optional<std::string> NamespaceNameAsWritten =
117+
getNamespaceNameAsWritten(LBraceLoc, Sources, getLangOpts());
118+
if (!NamespaceNameAsWritten)
119+
return;
120+
121+
if (NamespaceNameAsWritten->empty() != ND->isAnonymousNamespace()) {
122+
// Apparently, we didn't find the correct namespace name. Give up.
123+
return;
104124
}
105125

106-
// FIXME: This probably breaks on comments between the namespace and its '{'.
107-
auto TextRange =
108-
Lexer::getAsCharRange(SourceRange(NestedNamespaceBegin, LBracketLocation),
109-
Sources, getLangOpts());
110-
StringRef NestedNamespaceName =
111-
Lexer::getSourceText(TextRange, Sources, getLangOpts())
112-
.rtrim('{') // Drop the { itself.
113-
.rtrim(); // Drop any whitespace before it.
114-
bool IsNested = NestedNamespaceName.contains(':');
115-
116-
if (IsNested)
117-
Ends.push_back(LBracketLocation);
118-
else
119-
NestedNamespaceName = ND->getName();
126+
Ends.push_back(LBraceLoc);
120127

128+
Token Tok;
121129
// Skip whitespace until we find the next token.
122130
while (Lexer::getRawToken(Loc, Tok, Sources, getLangOpts()) ||
123131
Tok.is(tok::semi)) {
@@ -143,13 +151,9 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
143151
StringRef NamespaceNameInComment = Groups.size() > 5 ? Groups[5] : "";
144152
StringRef Anonymous = Groups.size() > 3 ? Groups[3] : "";
145153

146-
if (IsNested && NestedNamespaceName == NamespaceNameInComment) {
147-
// C++17 nested namespace.
148-
return;
149-
} else if ((ND->isAnonymousNamespace() &&
150-
NamespaceNameInComment.empty()) ||
151-
(ND->getNameAsString() == NamespaceNameInComment &&
152-
Anonymous.empty())) {
154+
if ((ND->isAnonymousNamespace() && NamespaceNameInComment.empty()) ||
155+
(*NamespaceNameAsWritten == NamespaceNameInComment &&
156+
Anonymous.empty())) {
153157
// Check if the namespace in the comment is the same.
154158
// FIXME: Maybe we need a strict mode, where we always fix namespace
155159
// comments with different format.
@@ -177,27 +181,29 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
177181
// multi-line or there may be other tokens behind it.
178182
}
179183

180-
std::string NamespaceName =
181-
ND->isAnonymousNamespace()
182-
? "anonymous namespace"
183-
: ("namespace '" + NestedNamespaceName.str() + "'");
184+
std::string NamespaceNameForDiag =
185+
ND->isAnonymousNamespace() ? "anonymous namespace"
186+
: ("namespace '" + *NamespaceNameAsWritten + "'");
187+
188+
std::string Fix(SpacesBeforeComments, ' ');
189+
Fix.append("// namespace");
190+
if (!ND->isAnonymousNamespace())
191+
Fix.append(" ").append(*NamespaceNameAsWritten);
192+
if (NeedLineBreak)
193+
Fix.append("\n");
184194

185195
// Place diagnostic at an old comment, or closing brace if we did not have it.
186196
SourceLocation DiagLoc =
187197
OldCommentRange.getBegin() != OldCommentRange.getEnd()
188198
? OldCommentRange.getBegin()
189199
: ND->getRBraceLoc();
190200

191-
diag(DiagLoc, Message)
192-
<< NamespaceName
193-
<< FixItHint::CreateReplacement(
194-
CharSourceRange::getCharRange(OldCommentRange),
195-
std::string(SpacesBeforeComments, ' ') +
196-
(IsNested
197-
? getNamespaceComment(NestedNamespaceName, NeedLineBreak)
198-
: getNamespaceComment(ND, NeedLineBreak)));
201+
diag(DiagLoc, Message) << NamespaceNameForDiag
202+
<< FixItHint::CreateReplacement(
203+
CharSourceRange::getCharRange(OldCommentRange),
204+
Fix);
199205
diag(ND->getLocation(), "%0 starts here", DiagnosticIDs::Note)
200-
<< NamespaceName;
206+
<< NamespaceNameForDiag;
201207
}
202208

203209
} // namespace readability

clang-tools-extra/test/clang-tidy/checkers/google-readability-nested-namespace-comments.cpp renamed to clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments-c++17.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,17 @@
11
// RUN: %check_clang_tidy %s google-readability-namespace-comments %t
22

33
namespace n1::n2 {
4-
namespace n3 {
4+
namespace /*comment1*/n3/*comment2*/::/*comment3*/inline/*comment4*/n4/*comment5*/ {
55

66
// So that namespace is not empty.
77
void f();
88

99

10-
// CHECK-MESSAGES: :[[@LINE+4]]:1: warning: namespace 'n3' not terminated with
11-
// CHECK-MESSAGES: :[[@LINE-7]]:11: note: namespace 'n3' starts here
10+
// CHECK-MESSAGES: :[[@LINE+4]]:1: warning: namespace 'n3::n4' not terminated with
11+
// CHECK-MESSAGES: :[[@LINE-7]]:23: note: namespace 'n3::n4' starts here
1212
// CHECK-MESSAGES: :[[@LINE+2]]:2: warning: namespace 'n1::n2' not terminated with a closing comment [google-readability-namespace-comments]
1313
// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'n1::n2' starts here
1414
}}
15-
// CHECK-FIXES: } // namespace n3
15+
// CHECK-FIXES: } // namespace n3::n4
1616
// CHECK-FIXES: } // namespace n1::n2
1717

‎clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp‎

Lines changed: 63 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
// RUN: %check_clang_tidy %s google-readability-namespace-comments %t
22

33
namespace n1 {
4-
namespace n2 {
4+
namespace /* a comment */ n2 /* another comment */ {
55

66

77
void f(); // So that the namespace isn't empty.
88

99

1010
// CHECK-MESSAGES: :[[@LINE+4]]:1: warning: namespace 'n2' not terminated with a closing comment [google-readability-namespace-comments]
11-
// CHECK-MESSAGES: :[[@LINE-7]]:11: note: namespace 'n2' starts here
11+
// CHECK-MESSAGES: :[[@LINE-7]]:27: note: namespace 'n2' starts here
1212
// CHECK-MESSAGES: :[[@LINE+2]]:2: warning: namespace 'n1' not terminated with
1313
// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'n1' starts here
1414
}}
@@ -25,11 +25,72 @@ void f(); // So that the namespace isn't empty.
2525
// 5
2626
// 6
2727
// 7
28+
// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'MACRO' not terminated with
29+
// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'MACRO' starts here
30+
}
31+
// CHECK-FIXES: } // namespace MACRO
32+
33+
namespace macro_expansion {
34+
void ff(); // So that the namespace isn't empty.
35+
// 1
36+
// 2
37+
// 3
38+
// 4
39+
// 5
40+
// 6
41+
// 7
2842
// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'macro_expansion' not terminated with
2943
// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'macro_expansion' starts here
3044
}
3145
// CHECK-FIXES: } // namespace macro_expansion
3246

47+
namespace [[deprecated("foo")]] namespace_with_attr {
48+
inline namespace inline_namespace {
49+
void g();
50+
// 1
51+
// 2
52+
// 3
53+
// 4
54+
// 5
55+
// 6
56+
// 7
57+
// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'inline_namespace' not terminated with
58+
// CHECK-MESSAGES: :[[@LINE-10]]:18: note: namespace 'inline_namespace' starts here
59+
}
60+
// CHECK-FIXES: } // namespace inline_namespace
61+
// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'namespace_with_attr' not terminated with
62+
// CHECK-MESSAGES: :[[@LINE-15]]:33: note: namespace 'namespace_with_attr' starts here
63+
}
64+
// CHECK-FIXES: } // namespace namespace_with_attr
65+
66+
namespace [[deprecated]] {
67+
void h();
68+
// 1
69+
// 2
70+
// 3
71+
// 4
72+
// 5
73+
// 6
74+
// 7
75+
// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: anonymous namespace not terminated with
76+
// CHECK-MESSAGES: :[[@LINE-10]]:26: note: anonymous namespace starts here
77+
}
78+
// CHECK-FIXES: } // namespace{{$}}
79+
80+
namespace [[]] {
81+
void hh();
82+
// 1
83+
// 2
84+
// 3
85+
// 4
86+
// 5
87+
// 6
88+
// 7
89+
// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: anonymous namespace not terminated with
90+
// CHECK-MESSAGES: :[[@LINE-10]]:16: note: anonymous namespace starts here
91+
}
92+
// CHECK-FIXES: } // namespace{{$}}
93+
3394
namespace short1 {
3495
namespace short2 {
3596
// Namespaces covering 10 lines or fewer are exempt from this rule.

0 commit comments

Comments
 (0)