HerrCai0907 updated this revision to Diff 509860.
HerrCai0907 marked an inline comment as done.
HerrCai0907 added a comment.
refactor some code and add release note
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147194/new/
https://reviews.llvm.org/D147194
Files:
clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.h
clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
clang-tools-extra/clang-tidy/utils/LexerUtils.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/concat-nested-namespaces/modernize-concat-nested-namespaces.h
clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp
@@ -156,6 +156,20 @@
} // namespace n41
// CHECK-FIXES: }
+
+namespace n43 {
+// CHECK-MESSAGES-DAG: :[[@LINE-1]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+#define N43_INNER
+namespace n44 {
+void foo() {}
+} // namespace n44
+#undef N43_INNER
+} // namespace n43
+// CHECK-FIXES: #define N43_INNER
+// CHECK-FIXES: namespace n43::n44 {
+// CHECK-FIXES: }
+// CHECK-FIXES: #undef N43_INNER
+
int main() {
n26::n27::n28::n29::n30::t();
#ifdef IEXIST
Index: clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/concat-nested-namespaces/modernize-concat-nested-namespaces.h
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/concat-nested-namespaces/modernize-concat-nested-namespaces.h
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/concat-nested-namespaces/modernize-concat-nested-namespaces.h
@@ -5,4 +5,4 @@
} // namespace nn2
} // namespace nn1
// CHECK-FIXES: void t();
-// CHECK-FIXES-NEXT: } // namespace nn1
+// CHECK-FIXES-NEXT: } // namespace nn2
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -261,6 +261,10 @@
<clang-tidy/checks/google/readability-avoid-underscore-in-googletest-name>` when using
``DISABLED_`` in the test suite name.
+- Fixed an issue in :doc:`modernize-concat-nested-namespaces
+ <clang-tidy/checks/modernize/concat-nested-namespaces>` when using macro between
+ namespace declarations could result incorrect fix.
+
Removed checks
^^^^^^^^^^^^^^
Index: clang-tools-extra/clang-tidy/utils/LexerUtils.h
===================================================================
--- clang-tools-extra/clang-tidy/utils/LexerUtils.h
+++ clang-tools-extra/clang-tidy/utils/LexerUtils.h
@@ -85,6 +85,10 @@
}
}
+std::optional<Token>
+findNextTokenIncludingComments(SourceLocation Start, const SourceManager &SM,
+ const LangOptions &LangOpts);
+
// Finds next token that's not a comment.
std::optional<Token> findNextTokenSkippingComments(SourceLocation Start,
const SourceManager &SM,
Index: clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
@@ -75,6 +75,29 @@
return findNextAnyTokenKind(Start, SM, LangOpts, tok::comma, tok::semi);
}
+std::optional<Token>
+findNextTokenIncludingComments(SourceLocation Start, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ // `Lexer::findNextToken` will ignore comment
+ if (Start.isMacroID())
+ return std::nullopt;
+ Start = Lexer::getLocForEndOfToken(Start, 0, SM, LangOpts);
+ // Break down the source location.
+ std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(Start);
+ bool InvalidTemp = false;
+ StringRef File = SM.getBufferData(LocInfo.first, &InvalidTemp);
+ if (InvalidTemp)
+ return std::nullopt;
+ // Lex from the start of the given location.
+ Lexer L(SM.getLocForStartOfFile(LocInfo.first), LangOpts, File.begin(),
+ File.data() + LocInfo.second, File.end());
+ L.SetCommentRetentionState(true);
+ // Find the token.
+ Token Tok;
+ L.LexFromRawLexer(Tok);
+ return Tok;
+}
+
std::optional<Token>
findNextTokenSkippingComments(SourceLocation Start, const SourceManager &SM,
const LangOptions &LangOpts) {
Index: clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.h
+++ clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.h
@@ -29,8 +29,8 @@
using NamespaceContextVec = llvm::SmallVector<const NamespaceDecl *, 6>;
using NamespaceString = llvm::SmallString<40>;
- void reportDiagnostic(const SourceRange &FrontReplacement,
- const SourceRange &BackReplacement);
+ void reportDiagnostic(const SourceManager &Sources,
+ const LangOptions &LangOpts);
NamespaceString concatNamespaces();
NamespaceContextVec Namespaces;
};
Index: clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
@@ -7,10 +7,12 @@
//===----------------------------------------------------------------------===//
#include "ConcatNestedNamespacesCheck.h"
+#include "../utils/LexerUtils.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Lex/Lexer.h"
+#include "clang/Basic/SourceLocation.h"
#include <algorithm>
+#include <optional>
namespace clang::tidy::modernize {
@@ -20,6 +22,13 @@
Sources.getFileID(Loc1) == Sources.getFileID(Loc2);
}
+static StringRef getRawStringRef(const SourceRange &Range,
+ const SourceManager &Sources,
+ const LangOptions &LangOpts) {
+ CharSourceRange TextRange = Lexer::getAsCharRange(Range, Sources, LangOpts);
+ return Lexer::getSourceText(TextRange, Sources, LangOpts);
+}
+
static bool anonymousOrInlineNamespace(const NamespaceDecl &ND) {
return ND.isAnonymousNamespace() || ND.isInlineNamespace();
}
@@ -38,11 +47,47 @@
const SourceManager &Sources,
const LangOptions &LangOpts) {
// FIXME: This logic breaks when there is a comment with ':'s in the middle.
- CharSourceRange TextRange =
- Lexer::getAsCharRange(ReplacementRange, Sources, LangOpts);
- StringRef CurrentNamespacesText =
- Lexer::getSourceText(TextRange, Sources, LangOpts);
- return CurrentNamespacesText.count(':') == (NumCandidates - 1) * 2;
+ return getRawStringRef(ReplacementRange, Sources, LangOpts).count(':') ==
+ (NumCandidates - 1) * 2;
+}
+
+static std::optional<SourceRange>
+getCleanedNamespaceFrontRange(const NamespaceDecl *ND, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ // Front from namespace tp '{'
+ std::optional<Token> Tok =
+ ::clang::tidy::utils::lexer::findNextTokenSkippingComments(
+ ND->getLocation(), SM, LangOpts);
+ if (!Tok)
+ return std::nullopt;
+ while (Tok->getKind() != tok::TokenKind::l_brace) {
+ Tok = utils::lexer::findNextTokenSkippingComments(Tok->getEndLoc(), SM,
+ LangOpts);
+ if (!Tok)
+ return std::nullopt;
+ }
+ SourceRange{ND->getBeginLoc(), Tok->getEndLoc()}.dump(SM);
+ return SourceRange{ND->getBeginLoc(), Tok->getEndLoc()};
+}
+
+static SourceRange getCleanedNamespaceBackRange(const NamespaceDecl *ND,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ // Back from '}' to conditional '// namespace xxx'
+ const SourceRange DefaultSourceRange =
+ SourceRange{ND->getRBraceLoc(), ND->getRBraceLoc()};
+ SourceLocation Loc = ND->getRBraceLoc();
+ std::optional<Token> Tok =
+ utils::lexer::findNextTokenIncludingComments(Loc, SM, LangOpts);
+ if (!Tok)
+ return DefaultSourceRange;
+ if (Tok->getKind() != tok::TokenKind::comment)
+ return DefaultSourceRange;
+ SourceRange TokRange = SourceRange{Tok->getLocation(), Tok->getEndLoc()};
+ StringRef TokText = getRawStringRef(TokRange, SM, LangOpts);
+ if (TokText != "// namespace " + ND->getNameAsString())
+ return DefaultSourceRange;
+ return SourceRange{ND->getRBraceLoc(), Tok->getEndLoc()};
}
ConcatNestedNamespacesCheck::NamespaceString
@@ -65,11 +110,47 @@
}
void ConcatNestedNamespacesCheck::reportDiagnostic(
- const SourceRange &FrontReplacement, const SourceRange &BackReplacement) {
- diag(Namespaces.front()->getBeginLoc(),
- "nested namespaces can be concatenated", DiagnosticIDs::Warning)
- << FixItHint::CreateReplacement(FrontReplacement, concatNamespaces())
- << FixItHint::CreateReplacement(BackReplacement, "}");
+ const SourceManager &SM, const LangOptions &LangOpts) {
+ DiagnosticBuilder DB =
+ diag(Namespaces.front()->getBeginLoc(),
+ "nested namespaces can be concatenated", DiagnosticIDs::Warning);
+
+ SmallVector<SourceRange, 6> Fronts;
+ Fronts.reserve(Namespaces.size() - 1U);
+ SmallVector<SourceRange, 6> Backs;
+ Backs.reserve(Namespaces.size());
+
+ NamespaceDecl const *ND = nullptr; // ND is the last non-nest namespace
+
+ for (size_t Index = 0; Index < Namespaces.size(); Index++) {
+ if (Namespaces[Index]->isNested()) {
+ continue;
+ }
+ ND = Namespaces[Index];
+ std::optional<SourceRange> SR =
+ getCleanedNamespaceFrontRange(ND, SM, LangOpts);
+ if (!SR.has_value()) {
+ return;
+ }
+ Fronts.push_back(SR.value());
+ Backs.push_back(getCleanedNamespaceBackRange(ND, SM, LangOpts));
+ }
+ if (ND == nullptr || Fronts.empty() || Backs.empty()) {
+ return;
+ }
+ // the last one should be handled specially
+ Fronts.pop_back();
+ Backs.pop_back();
+
+ for (SourceRange const &Front : Fronts) {
+ DB << FixItHint::CreateRemoval(Front);
+ }
+ for (SourceRange const &Back : Backs) {
+ DB << FixItHint::CreateRemoval(Back);
+ }
+
+ DB << FixItHint::CreateReplacement(
+ SourceRange{ND->getBeginLoc(), ND->getLocation()}, concatNamespaces());
}
void ConcatNestedNamespacesCheck::check(
@@ -90,12 +171,10 @@
SourceRange FrontReplacement(Namespaces.front()->getBeginLoc(),
Namespaces.back()->getLocation());
- SourceRange BackReplacement(Namespaces.back()->getRBraceLoc(),
- Namespaces.front()->getRBraceLoc());
if (!alreadyConcatenated(Namespaces.size(), FrontReplacement, Sources,
getLangOpts()))
- reportDiagnostic(FrontReplacement, BackReplacement);
+ reportDiagnostic(Sources, getLangOpts());
Namespaces.clear();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
