diff --git a/clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.h b/clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.h --- a/clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.h @@ -29,8 +29,8 @@ using NamespaceContextVec = llvm::SmallVector; 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; }; diff --git a/clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp b/clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp --- a/clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp +++ b/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 +#include 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 +getCleanedNamespaceFrontRange(const NamespaceDecl *ND, const SourceManager &SM, + const LangOptions &LangOpts) { + // Front from namespace tp '{' + std::optional 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 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 Fronts; + Fronts.reserve(Namespaces.size() - 1U); + SmallVector 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 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(); } diff --git a/clang-tools-extra/clang-tidy/utils/LexerUtils.h b/clang-tools-extra/clang-tidy/utils/LexerUtils.h --- a/clang-tools-extra/clang-tidy/utils/LexerUtils.h +++ b/clang-tools-extra/clang-tidy/utils/LexerUtils.h @@ -85,6 +85,10 @@ } } +std::optional +findNextTokenIncludingComments(SourceLocation Start, const SourceManager &SM, + const LangOptions &LangOpts); + // Finds next token that's not a comment. std::optional findNextTokenSkippingComments(SourceLocation Start, const SourceManager &SM, diff --git a/clang-tools-extra/clang-tidy/utils/LexerUtils.cpp b/clang-tools-extra/clang-tidy/utils/LexerUtils.cpp --- a/clang-tools-extra/clang-tidy/utils/LexerUtils.cpp +++ b/clang-tools-extra/clang-tidy/utils/LexerUtils.cpp @@ -75,6 +75,29 @@ return findNextAnyTokenKind(Start, SM, LangOpts, tok::comma, tok::semi); } +std::optional +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 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 findNextTokenSkippingComments(SourceLocation Start, const SourceManager &SM, const LangOptions &LangOpts) { diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -261,6 +261,10 @@ ` when using ``DISABLED_`` in the test suite name. +- Fixed an issue in :doc:`modernize-concat-nested-namespaces + ` when using macro between + namespace declarations could result incorrect fix. + Removed checks ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/concat-nested-namespaces/modernize-concat-nested-namespaces.h b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/concat-nested-namespaces/modernize-concat-nested-namespaces.h --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/concat-nested-namespaces/modernize-concat-nested-namespaces.h +++ b/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 diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/concat-nested-namespaces.cpp +++ b/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