HerrCai0907 updated this revision to Diff 511757.
HerrCai0907 added a comment.
Herald added a subscriber: ChuanqiXu.
update according to comment
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
@@ -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();
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 nn1::nn2
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -297,6 +297,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.
+
- Fixed a false positive in :doc:`performance-no-automatic-move
<clang-tidy/checks/performance/no-automatic-move>` when warning would be
emitted for a const local variable to which NRVO is applied.
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,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 <algorithm>
+#include <optional>
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<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;
+ }
+ 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);
+ 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<SourceRange, 6> Fronts;
+ Fronts.reserve(Namespaces.size() - 1U);
+ SmallVector<SourceRange, 6> Backs;
+ Backs.reserve(Namespaces.size());
+
+ NamespaceDecl const *LastND = nullptr;
+
+ for (const NamespaceDecl *ND : Namespaces) {
+ if (ND->isNested())
+ continue;
+ LastND = ND;
+ 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 (LastND == nullptr || Fronts.empty() || Backs.empty())
+ return;
+ // the last one should be handled specially
+ Fronts.pop_back();
+ SourceRange LastRBrace = Backs.pop_back_val();
+
+ for (SourceRange const &Front : Fronts)
+ DB << FixItHint::CreateRemoval(Front);
+ for (SourceRange const &Back : llvm::reverse(Backs))
+ DB << FixItHint::CreateRemoval(Back);
+ NamespaceString ConcatNameSpace = concatNamespaces();
+ DB << FixItHint::CreateReplacement(
+ SourceRange{LastND->getBeginLoc(), LastND->getLocation()},
+ ConcatNameSpace);
+ if (LastRBrace != SourceRange{LastND->getRBraceLoc(), LastND->getRBraceLoc()})
+ DB << FixItHint::CreateReplacement(LastRBrace,
+ ("} // " + ConcatNameSpace).str());
}
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();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
