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,14 @@ //===----------------------------------------------------------------------===// #include "ConcatNestedNamespacesCheck.h" +#include "../utils/LexerUtils.h" #include "clang/AST/ASTContext.h" +#include "clang/AST/Decl.h" #include "clang/ASTMatchers/ASTMatchFinder.h" -#include "clang/Lex/Lexer.h" +#include "clang/Basic/SourceLocation.h" +#include "llvm/ADT/STLExtras.h" #include +#include namespace clang::tidy::modernize { @@ -20,6 +24,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 +49,49 @@ 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; + } + 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); + std::string CloseComment = "namespace " + ND->getNameAsString(); + // current fix hint in readability/NamespaceCommentCheck.cpp use single line + // comment + if (TokText != "// " + CloseComment && TokText != "//" + CloseComment) + return DefaultSourceRange; + return SourceRange{ND->getRBraceLoc(), Tok->getEndLoc()}; } ConcatNestedNamespacesCheck::NamespaceString @@ -65,11 +114,46 @@ } 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 *LastND = nullptr; + + for (const NamespaceDecl *ND : Namespaces) { + if (ND->isNested()) + continue; + LastND = ND; + 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 (LastND == nullptr || Fronts.empty() || Backs.empty()) + return; + // the last one should be handled specially + Fronts.pop_back(); + SourceRange LastRBrace = Backs.pop_back_val(); + NamespaceString ConcatNameSpace = concatNamespaces(); + + for (SourceRange const &Front : Fronts) + DB << FixItHint::CreateRemoval(Front); + DB << FixItHint::CreateReplacement( + SourceRange{LastND->getBeginLoc(), LastND->getLocation()}, + ConcatNameSpace); + if (LastRBrace != SourceRange{LastND->getRBraceLoc(), LastND->getRBraceLoc()}) + DB << FixItHint::CreateReplacement(LastRBrace, + ("} // " + ConcatNameSpace).str()); + for (SourceRange const &Back : llvm::reverse(Backs)) + DB << FixItHint::CreateRemoval(Back); } void ConcatNestedNamespacesCheck::check( @@ -90,12 +174,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 @@ -297,6 +297,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. + - Fixed a false positive in :doc:`performance-no-automatic-move ` when warning would be emitted for a const local variable to which NRVO is applied. 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 nn1::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 @@ -143,7 +143,7 @@ #endif } // namespace n40 } // namespace n39 -// CHECK-FIXES: } +// CHECK-FIXES: } // namespace n39::n40 namespace n41 { namespace n42 { @@ -154,7 +154,59 @@ #endif } // namespace n42 } // namespace n41 -// CHECK-FIXES: } +// CHECK-FIXES: } // namespace n41::n42 + + +// CHECK-MESSAGES-DAG: :[[@LINE+1]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces] +namespace n43 { +#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: } // namespace n43::n44 +// CHECK-FIXES: #undef N43_INNER + +// CHECK-MESSAGES-DAG: :[[@LINE+1]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces] +namespace n45{ +#define N45_INNER +namespace n46 +{ +#pragma clang diagnostic push +namespace n47 { +void foo() {} +} // namespace n47 +#pragma clang diagnostic pop +} //namespace n46 +#undef N45_INNER +} //namespace n45 +// CHECK-FIXES: #define N45_INNER +// CHECK-FIXES: #pragma clang diagnostic push +// CHECK-FIXES: namespace n45::n46::n47 { +// CHECK-FIXES: } // namespace n45::n46::n47 +// CHECK-FIXES: #pragma clang diagnostic pop +// CHECK-FIXES: #undef N45_INNER + +// CHECK-MESSAGES-DAG: :[[@LINE+1]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces] +namespace avoid_add_close_comment { +namespace inner { +void foo() {} +} +} +// CHECK-FIXES: namespace avoid_add_close_comment::inner { +// CHECK-FIXES-NOT: } // namespace avoid_add_close_comment::inner + +// CHECK-MESSAGES-DAG: :[[@LINE+1]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces] +namespace avoid_change_close_comment { +namespace inner { +void foo() {} +} // namespace inner and other comments +} // namespace avoid_change_close_comment and other comments +// CHECK-FIXES: namespace avoid_change_close_comment::inner { +// CHECK-FIXES-NOT: } // namespace avoid_add_close_comment::inner int main() { n26::n27::n28::n29::n30::t();