[clang-tools-extra] fac4e3c - [clang-tidy] Fix PR26274
Alexander Kornienko via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 6 01:10:22 PST 2019
Author: Alexander Kornienko
Date: 2019-12-06T10:10:15+01:00
New Revision: fac4e3c5f8a018599cbd9363a735b1c13e8f8a05
URL: https://github.com/llvm/llvm-project/commit/fac4e3c5f8a018599cbd9363a735b1c13e8f8a05
DIFF: https://github.com/llvm/llvm-project/commit/fac4e3c5f8a018599cbd9363a735b1c13e8f8a05.diff
LOG: [clang-tidy] Fix PR26274
Summary:
This commit fixes http://llvm.org/PR26274 in a simpler and more correct way than
4736d63f752f8d13f4c6a9afd558565c32119718 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
Added:
clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments-c++17.cpp
Modified:
clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
Removed:
clang-tools-extra/test/clang-tidy/checkers/google-readability-nested-namespace-comments.cpp
################################################################################
diff --git a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
index eb3d7c505b83..4a7c4566230e 100644
--- a/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
@@ -7,9 +7,11 @@
//===----------------------------------------------------------------------===//
#include "NamespaceCommentCheck.h"
+#include "../utils/LexerUtils.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/TokenKinds.h"
#include "clang/Lex/Lexer.h"
#include "llvm/ADT/StringExtras.h"
@@ -46,30 +48,48 @@ static bool locationsInSameFile(const SourceManager &Sources,
Sources.getFileID(Loc1) == Sources.getFileID(Loc2);
}
-static std::string getNamespaceComment(const NamespaceDecl *ND,
- bool InsertLineBreak) {
- std::string Fix = "// namespace";
- if (!ND->isAnonymousNamespace())
- Fix.append(" ").append(ND->getNameAsString());
- if (InsertLineBreak)
- Fix.append("\n");
- return Fix;
-}
-
-static std::string getNamespaceComment(const std::string &NameSpaceName,
- bool InsertLineBreak) {
- std::string Fix = "// namespace ";
- Fix.append(NameSpaceName);
- if (InsertLineBreak)
- Fix.append("\n");
- return Fix;
+static llvm::Optional<std::string>
+getNamespaceNameAsWritten(SourceLocation &Loc, const SourceManager &Sources,
+ const LangOptions &LangOpts) {
+ // Loc should be at the begin of the namespace decl (usually, `namespace`
+ // token). We skip the first token right away, but in case of `inline
+ // namespace` or `namespace a::inline b` we can see both `inline` and
+ // `namespace` keywords, which we just ignore. Nested parens/squares before
+ // the opening brace can result from attributes.
+ std::string Result;
+ int Nesting = 0;
+ while (llvm::Optional<Token> T = utils::lexer::findNextTokenSkippingComments(
+ Loc, Sources, LangOpts)) {
+ Loc = T->getLocation();
+ if (T->is(tok::l_brace))
+ break;
+
+ if (T->isOneOf(tok::l_square, tok::l_paren)) {
+ ++Nesting;
+ } else if (T->isOneOf(tok::r_square, tok::r_paren)) {
+ --Nesting;
+ } else if (Nesting == 0) {
+ if (T->is(tok::raw_identifier)) {
+ StringRef ID = T->getRawIdentifier();
+ if (ID != "namespace" && ID != "inline")
+ Result.append(ID);
+ } else if (T->is(tok::coloncolon)) {
+ Result.append("::");
+ } else { // Any other kind of token is unexpected here.
+ return llvm::None;
+ }
+ }
+ }
+ return Result;
}
void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
const auto *ND = Result.Nodes.getNodeAs<NamespaceDecl>("namespace");
const SourceManager &Sources = *Result.SourceManager;
- if (!locationsInSameFile(Sources, ND->getBeginLoc(), ND->getRBraceLoc()))
+ // Ignore namespaces inside macros and namespaces split across files.
+ if (ND->getBeginLoc().isMacroID() ||
+ !locationsInSameFile(Sources, ND->getBeginLoc(), ND->getRBraceLoc()))
return;
// Don't require closing comments for namespaces spanning less than certain
@@ -80,44 +100,32 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
return;
// Find next token after the namespace closing brace.
- SourceLocation AfterRBrace = ND->getRBraceLoc().getLocWithOffset(1);
+ SourceLocation AfterRBrace = Lexer::getLocForEndOfToken(
+ ND->getRBraceLoc(), /*Offset=*/0, Sources, getLangOpts());
SourceLocation Loc = AfterRBrace;
- Token Tok;
- SourceLocation LBracketLocation = ND->getLocation();
- SourceLocation NestedNamespaceBegin = LBracketLocation;
+ SourceLocation LBraceLoc = ND->getBeginLoc();
// Currently for nested namepsace (n1::n2::...) the AST matcher will match foo
// then bar instead of a single match. So if we got a nested namespace we have
// to skip the next ones.
for (const auto &EndOfNameLocation : Ends) {
- if (Sources.isBeforeInTranslationUnit(NestedNamespaceBegin,
- EndOfNameLocation))
+ if (Sources.isBeforeInTranslationUnit(ND->getLocation(), EndOfNameLocation))
return;
}
- // Ignore macros
- if (!ND->getLocation().isMacroID()) {
- while (Lexer::getRawToken(LBracketLocation, Tok, Sources, getLangOpts()) ||
- !Tok.is(tok::l_brace)) {
- LBracketLocation = LBracketLocation.getLocWithOffset(1);
- }
+ llvm::Optional<std::string> NamespaceNameAsWritten =
+ getNamespaceNameAsWritten(LBraceLoc, Sources, getLangOpts());
+ if (!NamespaceNameAsWritten)
+ return;
+
+ if (NamespaceNameAsWritten->empty() != ND->isAnonymousNamespace()) {
+ // Apparently, we didn't find the correct namespace name. Give up.
+ return;
}
- // FIXME: This probably breaks on comments between the namespace and its '{'.
- auto TextRange =
- Lexer::getAsCharRange(SourceRange(NestedNamespaceBegin, LBracketLocation),
- Sources, getLangOpts());
- StringRef NestedNamespaceName =
- Lexer::getSourceText(TextRange, Sources, getLangOpts())
- .rtrim('{') // Drop the { itself.
- .rtrim(); // Drop any whitespace before it.
- bool IsNested = NestedNamespaceName.contains(':');
-
- if (IsNested)
- Ends.push_back(LBracketLocation);
- else
- NestedNamespaceName = ND->getName();
+ Ends.push_back(LBraceLoc);
+ Token Tok;
// Skip whitespace until we find the next token.
while (Lexer::getRawToken(Loc, Tok, Sources, getLangOpts()) ||
Tok.is(tok::semi)) {
@@ -143,13 +151,9 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
StringRef NamespaceNameInComment = Groups.size() > 5 ? Groups[5] : "";
StringRef Anonymous = Groups.size() > 3 ? Groups[3] : "";
- if (IsNested && NestedNamespaceName == NamespaceNameInComment) {
- // C++17 nested namespace.
- return;
- } else if ((ND->isAnonymousNamespace() &&
- NamespaceNameInComment.empty()) ||
- (ND->getNameAsString() == NamespaceNameInComment &&
- Anonymous.empty())) {
+ if ((ND->isAnonymousNamespace() && NamespaceNameInComment.empty()) ||
+ (*NamespaceNameAsWritten == NamespaceNameInComment &&
+ Anonymous.empty())) {
// Check if the namespace in the comment is the same.
// FIXME: Maybe we need a strict mode, where we always fix namespace
// comments with
diff erent format.
@@ -177,10 +181,16 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
// multi-line or there may be other tokens behind it.
}
- std::string NamespaceName =
- ND->isAnonymousNamespace()
- ? "anonymous namespace"
- : ("namespace '" + NestedNamespaceName.str() + "'");
+ std::string NamespaceNameForDiag =
+ ND->isAnonymousNamespace() ? "anonymous namespace"
+ : ("namespace '" + *NamespaceNameAsWritten + "'");
+
+ std::string Fix(SpacesBeforeComments, ' ');
+ Fix.append("// namespace");
+ if (!ND->isAnonymousNamespace())
+ Fix.append(" ").append(*NamespaceNameAsWritten);
+ if (NeedLineBreak)
+ Fix.append("\n");
// Place diagnostic at an old comment, or closing brace if we did not have it.
SourceLocation DiagLoc =
@@ -188,16 +198,12 @@ void NamespaceCommentCheck::check(const MatchFinder::MatchResult &Result) {
? OldCommentRange.getBegin()
: ND->getRBraceLoc();
- diag(DiagLoc, Message)
- << NamespaceName
- << FixItHint::CreateReplacement(
- CharSourceRange::getCharRange(OldCommentRange),
- std::string(SpacesBeforeComments, ' ') +
- (IsNested
- ? getNamespaceComment(NestedNamespaceName, NeedLineBreak)
- : getNamespaceComment(ND, NeedLineBreak)));
+ diag(DiagLoc, Message) << NamespaceNameForDiag
+ << FixItHint::CreateReplacement(
+ CharSourceRange::getCharRange(OldCommentRange),
+ Fix);
diag(ND->getLocation(), "%0 starts here", DiagnosticIDs::Note)
- << NamespaceName;
+ << NamespaceNameForDiag;
}
} // namespace readability
diff --git a/clang-tools-extra/test/clang-tidy/checkers/google-readability-nested-namespace-comments.cpp b/clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments-c++17.cpp
similarity index 58%
rename from clang-tools-extra/test/clang-tidy/checkers/google-readability-nested-namespace-comments.cpp
rename to clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments-c++17.cpp
index 017081d2e952..e4626020ebaa 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/google-readability-nested-namespace-comments.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments-c++17.cpp
@@ -1,17 +1,17 @@
// RUN: %check_clang_tidy %s google-readability-namespace-comments %t
namespace n1::n2 {
-namespace n3 {
+namespace /*comment1*/n3/*comment2*/::/*comment3*/inline/*comment4*/n4/*comment5*/ {
// So that namespace is not empty.
void f();
-// CHECK-MESSAGES: :[[@LINE+4]]:1: warning: namespace 'n3' not terminated with
-// CHECK-MESSAGES: :[[@LINE-7]]:11: note: namespace 'n3' starts here
+// CHECK-MESSAGES: :[[@LINE+4]]:1: warning: namespace 'n3::n4' not terminated with
+// CHECK-MESSAGES: :[[@LINE-7]]:23: note: namespace 'n3::n4' starts here
// CHECK-MESSAGES: :[[@LINE+2]]:2: warning: namespace 'n1::n2' not terminated with a closing comment [google-readability-namespace-comments]
// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'n1::n2' starts here
}}
-// CHECK-FIXES: } // namespace n3
+// CHECK-FIXES: } // namespace n3::n4
// CHECK-FIXES: } // namespace n1::n2
diff --git a/clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp b/clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
index 591c9dae5a74..963449a213e5 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
@@ -1,14 +1,14 @@
// RUN: %check_clang_tidy %s google-readability-namespace-comments %t
namespace n1 {
-namespace n2 {
+namespace /* a comment */ n2 /* another comment */ {
void f(); // So that the namespace isn't empty.
// CHECK-MESSAGES: :[[@LINE+4]]:1: warning: namespace 'n2' not terminated with a closing comment [google-readability-namespace-comments]
-// CHECK-MESSAGES: :[[@LINE-7]]:11: note: namespace 'n2' starts here
+// CHECK-MESSAGES: :[[@LINE-7]]:27: note: namespace 'n2' starts here
// CHECK-MESSAGES: :[[@LINE+2]]:2: warning: namespace 'n1' not terminated with
// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'n1' starts here
}}
@@ -25,11 +25,72 @@ void f(); // So that the namespace isn't empty.
// 5
// 6
// 7
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'MACRO' not terminated with
+// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'MACRO' starts here
+}
+// CHECK-FIXES: } // namespace MACRO
+
+namespace macro_expansion {
+void ff(); // So that the namespace isn't empty.
+// 1
+// 2
+// 3
+// 4
+// 5
+// 6
+// 7
// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'macro_expansion' not terminated with
// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'macro_expansion' starts here
}
// CHECK-FIXES: } // namespace macro_expansion
+namespace [[deprecated("foo")]] namespace_with_attr {
+inline namespace inline_namespace {
+void g();
+// 1
+// 2
+// 3
+// 4
+// 5
+// 6
+// 7
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'inline_namespace' not terminated with
+// CHECK-MESSAGES: :[[@LINE-10]]:18: note: namespace 'inline_namespace' starts here
+}
+// CHECK-FIXES: } // namespace inline_namespace
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: namespace 'namespace_with_attr' not terminated with
+// CHECK-MESSAGES: :[[@LINE-15]]:33: note: namespace 'namespace_with_attr' starts here
+}
+// CHECK-FIXES: } // namespace namespace_with_attr
+
+namespace [[deprecated]] {
+void h();
+// 1
+// 2
+// 3
+// 4
+// 5
+// 6
+// 7
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: anonymous namespace not terminated with
+// CHECK-MESSAGES: :[[@LINE-10]]:26: note: anonymous namespace starts here
+}
+// CHECK-FIXES: } // namespace{{$}}
+
+namespace [[]] {
+void hh();
+// 1
+// 2
+// 3
+// 4
+// 5
+// 6
+// 7
+// CHECK-MESSAGES: :[[@LINE+2]]:1: warning: anonymous namespace not terminated with
+// CHECK-MESSAGES: :[[@LINE-10]]:16: note: anonymous namespace starts here
+}
+// CHECK-FIXES: } // namespace{{$}}
+
namespace short1 {
namespace short2 {
// Namespaces covering 10 lines or fewer are exempt from this rule.
More information about the cfe-commits
mailing list