alexfh created this revision.
alexfh added reviewers: gribozavr, aaron.ballman.
Herald added a subscriber: xazax.hun.
Herald added a project: clang.
This commit fixes http://llvm.org/PR26274 in a simpler and more correct way than
4736d63f752f8d13f4c6a9afd558565c32119718
<https://reviews.llvm.org/rG4736d63f752f8d13f4c6a9afd558565c32119718> did. See
https://reviews.llvm.org/D69855#1767089 for details.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D70974
Files:
clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/google-readability-namespace-comments.cpp
@@ -25,6 +25,20 @@
// 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
}
Index: clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/NamespaceCommentCheck.cpp
+++ 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,13 @@
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;
-}
-
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 accross 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 +65,40 @@
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);
+ std::string NamespaceNameAsWritten;
+ while (llvm::Optional<Token> T = utils::lexer::findNextTokenSkippingComments(
+ LBraceLoc, Sources, getLangOpts())) {
+ LBraceLoc = T->getLocation();
+ if (T->is(tok::raw_identifier)) {
+ NamespaceNameAsWritten.append(T->getRawIdentifier());
+ } else if (T->is(tok::coloncolon)) {
+ NamespaceNameAsWritten.append("::");
+ } else {
+ break;
}
}
- // 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();
+ if (NamespaceNameAsWritten.empty() != ND->isAnonymousNamespace()) {
+ // Apparently, we didn't find the correct namespace name. Give up.
+ return;
+ }
+ 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 +124,9 @@
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 different format.
@@ -177,10 +154,16 @@
// 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 +171,12 @@
? 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
