JonasToth updated this revision to Diff 165749.
JonasToth marked an inline comment as done.
JonasToth added a comment.
- improve diagnostic to include number of declared variables
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51949
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/IsolateDeclCheck.cpp
clang-tidy/readability/IsolateDeclCheck.h
clang-tidy/readability/ReadabilityTidyModule.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-isolate-decl.rst
test/clang-tidy/readability-isolate-decl-cxx17.cpp
test/clang-tidy/readability-isolate-decl.cpp
Index: test/clang-tidy/readability-isolate-decl.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/readability-isolate-decl.cpp
@@ -0,0 +1,121 @@
+// RUN: %check_clang_tidy %s readability-isolate-decl %t
+
+void f() {
+ int i;
+}
+
+void f2() {
+ int i, j, *k, lala = 42;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: this statement declares 4 variables
+ // CHECK-FIXES: int i;
+ // CHECK-FIXES: {{^ }}int j;
+ // CHECK-FIXES: {{^ }}int *k;
+ // CHECK-FIXES: {{^ }}int lala = 42;
+
+ int normal, weird = /* comment */ 42;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: this statement declares 2 variables
+ // CHECK-FIXES: int normal;
+ // CHECK-FIXES: {{^ }}int weird = /* comment */ 42;
+ //
+ int /* here is a comment */ v1,
+ // another comment
+ v2 = 42 // Ok, more comments
+ ;
+ // CHECK-MESSAGES: [[@LINE-4]]:3: warning: this statement declares 2 variables
+ // CHECK-FIXES: int /* here is a comment */ v1;
+ // CHECK-FIXES: {{^ }}int /* here is a comment */ // another comment
+ // CHECK-FIXES: {{^ }}v2 = 42 // Ok, more comments
+ // CHECK-FIXES: {{^ }};
+}
+
+void f3() {
+ int i, *pointer1;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: this statement declares 2 variables
+ // CHECK-FIXES: int i;
+ // CHECK-FIXES: {{^ }}int *pointer1;
+ //
+ int *pointer2 = nullptr, *pointer3 = &i;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: this statement declares 2 variables
+ // CHECK-FIXES: int *pointer2 = nullptr;
+ // CHECK-FIXES: {{^ }}int *pointer3 = &i;
+}
+
+void f4() {
+ double d = 42. /* foo */, z = 43., /* hi */ y, c /* */ /* */, l = 2.;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: this statement declares 5 variables
+ // CHECK-FIXES: double d = 42. /* foo */;
+ // CHECK-FIXES: {{^ }}double z = 43.;
+ // CHECK-FIXES: {{^ }}double /* hi */ y;
+ // CHECK-FIXES: {{^ }}double c /* */ /* */;
+ // CHECK-FIXES: {{^ }}double l = 2.;
+}
+
+struct SomeClass {
+ SomeClass() = default;
+ SomeClass(int value);
+};
+void f5() {
+ SomeClass v1, v2(42), v3{42}, v4(42.5);
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: this statement declares 4 variables
+ // CHECK-FIXES: SomeClass v1;
+ // CHECK-FIXES: {{^ }}SomeClass v2(42);
+ // CHECK-FIXES: {{^ }}SomeClass v3{42};
+ // CHECK-FIXES: {{^ }}SomeClass v4(42.5);
+
+ SomeClass v5 = 42, *p1 = nullptr;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: this statement declares 2 variables
+ // CHECK-FIXES: SomeClass v5 = 42;
+ // CHECK-FIXES: {{^ }}SomeClass *p1 = nullptr;
+}
+
+void f6() {
+ int array1[] = {1, 2, 3, 4}, array2[] = {1, 2, 3}, value1, value2 = 42;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: this statement declares 4 variables
+ // CHECK-FIXES: int array1[] = {1, 2, 3, 4};
+ // CHECK-FIXES: {{^ }}int array2[] = {1, 2, 3};
+ // CHECK-FIXES: {{^ }}int value1;
+ // CHECK-FIXES: {{^ }}int value2 = 42;
+}
+
+template <typename T>
+struct TemplatedType {
+ TemplatedType() = default;
+ TemplatedType(T value);
+};
+
+void f7() {
+ TemplatedType<int> TT1(42), TT2{42}, TT3;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: this statement declares 3 variables
+ // CHECK-FIXES: TemplatedType<int> TT1(42);
+ // CHECK-FIXES: {{^ }}TemplatedType<int> TT2{42};
+ // CHECK-FIXES: {{^ }}TemplatedType<int> TT3;
+}
+
+void forbidden_transformations() {
+ for (int i = 0, j = 42; i < j; ++i)
+ ;
+}
+
+#define NULL 0
+#define MY_NICE_TYPE int **
+#define VAR_NAME(name) name##__LINE__
+#define A_BUNCH_OF_VARIABLES int m1 = 42, m2 = 43, m3 = 44;
+
+void macros() {
+ int *p1 = NULL, *p2 = NULL;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: this statement declares 2 variables
+ // CHECK-FIXES: int *p1 = NULL;
+ // CHECK-FIXES: {{^ }}int *p2 = NULL;
+
+ // Macros are involved, so there will be no transformation
+ MY_NICE_TYPE p3, v1, v2;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: this statement declares 3 variables
+
+ int VAR_NAME(v3),
+ VAR_NAME(v4),
+ VAR_NAME(v5);
+ // CHECK-MESSAGES: [[@LINE-3]]:3: warning: this statement declares 3 variables
+
+ A_BUNCH_OF_VARIABLES
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: this statement declares 3 variables
+}
Index: test/clang-tidy/readability-isolate-decl-cxx17.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/readability-isolate-decl-cxx17.cpp
@@ -0,0 +1,37 @@
+// RUN: %check_clang_tidy %s readability-isolate-decl %t -- -- -std=c++17
+
+template <typename T1, typename T2>
+struct pair {
+ T1 first;
+ T2 second;
+ pair(T1 v1, T2 v2) : first(v1), second(v2) {}
+
+ template <int N>
+ decltype(auto) get() const {
+ if constexpr (N == 0)
+ return first;
+ else if constexpr (N == 1)
+ return second;
+ }
+};
+
+void forbidden_transformations() {
+ if (int i = 42, j = i; i == j)
+ ;
+
+ auto [i, j] = pair<int, int>(42, 42);
+}
+
+struct SomeClass {
+ SomeClass() = default;
+ SomeClass(int value);
+};
+
+void silence_tester() {
+ SomeClass v1, v2(42), v3{42}, v4(42.5);
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: this statement declares 4 variables
+ // CHECK-FIXES: SomeClass v1;
+ // CHECK-FIXES: {{^ }}SomeClass v2(42);
+ // CHECK-FIXES: {{^ }}SomeClass v3{42};
+ // CHECK-FIXES: {{^ }}SomeClass v4(42.5);
+}
Index: docs/clang-tidy/checks/readability-isolate-decl.rst
===================================================================
--- /dev/null
+++ docs/clang-tidy/checks/readability-isolate-decl.rst
@@ -0,0 +1,6 @@
+.. title:: clang-tidy - readability-isolate-decl
+
+readability-isolate-decl
+========================
+
+FIXME: Describe what patterns does the check detect and why. Give examples.
Index: docs/clang-tidy/checks/list.rst
===================================================================
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -225,6 +225,7 @@
readability-identifier-naming
readability-implicit-bool-conversion
readability-inconsistent-declaration-parameter-name
+ readability-isolate-decl
readability-magic-numbers
readability-misleading-indentation
readability-misplaced-array-index
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -93,6 +93,11 @@
Flags uses of ``absl::StrCat()`` to append to a ``std::string``. Suggests
``absl::StrAppend()`` should be used instead.
+- New :doc:`readability-isolate-decl
+ <clang-tidy/checks/readability-isolate-decl>` check.
+
+ FIXME: add release notes.
+
- New :doc:`readability-magic-numbers
<clang-tidy/checks/readability-magic-numbers>` check.
Index: clang-tidy/readability/ReadabilityTidyModule.cpp
===================================================================
--- clang-tidy/readability/ReadabilityTidyModule.cpp
+++ clang-tidy/readability/ReadabilityTidyModule.cpp
@@ -20,6 +20,7 @@
#include "IdentifierNamingCheck.h"
#include "ImplicitBoolConversionCheck.h"
#include "InconsistentDeclarationParameterNameCheck.h"
+#include "IsolateDeclCheck.h"
#include "MagicNumbersCheck.h"
#include "MisleadingIndentationCheck.h"
#include "MisplacedArrayIndexCheck.h"
@@ -66,6 +67,8 @@
"readability-implicit-bool-conversion");
CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>(
"readability-inconsistent-declaration-parameter-name");
+ CheckFactories.registerCheck<IsolateDeclCheck>(
+ "readability-isolate-decl");
CheckFactories.registerCheck<MagicNumbersCheck>(
"readability-magic-numbers");
CheckFactories.registerCheck<MisleadingIndentationCheck>(
Index: clang-tidy/readability/IsolateDeclCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/readability/IsolateDeclCheck.h
@@ -0,0 +1,35 @@
+//===--- IsolateDeclCheck.h - clang-tidy-------------------------*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ISOLATEDECLCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ISOLATEDECLCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// FIXME: Write a short description.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-isolate-decl.html
+class IsolateDeclCheck : public ClangTidyCheck {
+public:
+ IsolateDeclCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ISOLATEDECLCHECK_H
Index: clang-tidy/readability/IsolateDeclCheck.cpp
===================================================================
--- /dev/null
+++ clang-tidy/readability/IsolateDeclCheck.cpp
@@ -0,0 +1,282 @@
+//===--- IsolateDeclCheck.cpp - clang-tidy---------------------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "IsolateDeclCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/Support/Error.h"
+
+#include <iostream>
+#include <numeric>
+
+using namespace clang::ast_matchers;
+
+#define PRINT_DEBUG 0
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+namespace {
+AST_MATCHER(DeclStmt, isSingleDecl) { return Node.isSingleDecl(); }
+AST_MATCHER(DeclStmt, isVariablesOnly) {
+ return !std::any_of(Node.decl_begin(), Node.decl_end(),
+ [](Decl *D) { return dyn_cast<VarDecl>(D) == nullptr; });
+}
+
+AST_MATCHER_P(IfStmt, hasInit, ast_matchers::internal::Matcher<Stmt>,
+ InnerMatcher) {
+ const Stmt *const InitStmt = Node.getInit();
+ if (InitStmt)
+ return InnerMatcher.matches(*InitStmt, Finder, Builder);
+ return false;
+}
+} // namespace
+
+void IsolateDeclCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ stmt(allOf(
+ declStmt(allOf(isVariablesOnly(), unless(isSingleDecl())))
+ .bind("decl_stmt"),
+ unless(
+ hasAncestor(forStmt(hasLoopInit(equalsBoundNode("decl_stmt"))))),
+ unless(hasAncestor(ifStmt(hasInit(equalsBoundNode("decl_stmt"))))))),
+ this);
+}
+
+namespace {
+
+template <typename TokenKind, typename... TokenKinds>
+SourceLocation findNextAnyTokenKind(SourceLocation Start,
+ const SourceManager &SM,
+ const LangOptions &LangOpts, TokenKind TK,
+ TokenKinds... TKs) {
+ while (true) {
+ Optional<Token> CurrentToken = Lexer::findNextToken(Start, SM, LangOpts);
+
+ if (!CurrentToken.hasValue())
+ return SourceLocation();
+
+ Token PotentialMatch = *CurrentToken;
+ if (PotentialMatch.isOneOf(TK, TKs...))
+ return PotentialMatch.getLocation();
+
+ Start =
+ Lexer::getLocForEndOfToken(Start, 0, SM, LangOpts).getLocWithOffset(1);
+ }
+}
+
+SourceLocation findNextTerminator(SourceLocation Start, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ return findNextAnyTokenKind(Start, SM, LangOpts, tok::comma, tok::semi);
+}
+
+SourceLocation findStartOfIndirection(SourceLocation Start,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ return findNextAnyTokenKind(Start, SM, LangOpts, tok::star, tok::amp);
+}
+
+bool isMacroID(SourceRange R) {
+ return R.getBegin().isMacroID() || R.getEnd().isMacroID();
+}
+
+/// This function assumes, that \p DS only contains VarDecl elements.
+Optional<std::vector<SourceRange>> DeclSlice(const DeclStmt *DS,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ std::size_t DeclCount = std::distance(DS->decl_begin(), DS->decl_end());
+ if (DeclCount < 2) {
+#if PRINT_DEBUG
+ std::cerr << "Not enough Declarations (" << DeclCount
+ << ") in statement to create isolated declarations\n";
+#endif
+ return None;
+ }
+
+ // The initial type of the declaration and each declaration has it's slice.
+ std::vector<SourceRange> Slices;
+ Slices.reserve(DeclCount + 1);
+
+#if PRINT_DEBUG
+ std::cerr << "Number of Slices to create: " << Slices.capacity() << "\n";
+#endif
+
+ // Special case for the type where the SourceRange is computed
+ // differently.
+ VarDecl *FirstDecl = dyn_cast<VarDecl>(*DS->decl_begin());
+ assert(FirstDecl && "Expect only VarDecls");
+
+ SourceLocation Start;
+ if (FirstDecl->getType()->isPointerType() ||
+ FirstDecl->getType()->isReferenceType())
+ Start = findStartOfIndirection(DS->getBeginLoc(), SM, LangOpts);
+ else
+ Start = FirstDecl->getLocation();
+
+#if PRINT_DEBUG
+ std::cerr << "Start of Indirection: " << Start.printToString(SM) << "\n";
+#endif
+
+ SourceRange DeclRange(DS->getBeginLoc(), Start);
+ if (DeclRange.isInvalid()) {
+#if PRINT_DEBUG
+ std::cerr << "DeclRange is invalid\n";
+#endif
+ return None;
+ }
+ if (isMacroID(DeclRange)) {
+#if PRINT_DEBUG
+ std::cerr << "DeclRange is in macro\n";
+#endif
+ return None;
+ }
+
+ Slices.emplace_back(DeclRange);
+
+ SourceLocation DeclBegin = Start;
+ for (auto It = DS->decl_begin(), End = DS->decl_end(); It != End; ++It) {
+ VarDecl *CurrentDecl = dyn_cast<VarDecl>(*It);
+ assert(CurrentDecl && "Expect only VarDecls here");
+
+ SourceLocation DeclEnd;
+ if (CurrentDecl->hasInit())
+ DeclEnd =
+ findNextTerminator(CurrentDecl->getInit()->getEndLoc(), SM, LangOpts);
+ else
+ DeclEnd = findNextTerminator(CurrentDecl->getEndLoc(), SM, LangOpts);
+
+#if PRINT_DEBUG
+ std::cerr << "Start of Slice: " << DeclBegin.printToString(SM) << "\n";
+ std::cerr << "End of Slice: " << DeclEnd.printToString(SM) << "\n";
+#endif
+
+ SourceRange VarNameRange(DeclBegin, DeclEnd);
+ if (VarNameRange.isInvalid()) {
+#if PRINT_DEBUG
+ std::cerr << "VarNameRange is invalid\n";
+#endif
+ return None;
+ }
+ if (isMacroID(VarNameRange)) {
+#if PRINT_DEBUG
+ std::cerr << "VarNameRange is in macro\n";
+#endif
+ return None;
+ }
+
+ Slices.emplace_back(VarNameRange);
+ DeclBegin = DeclEnd.getLocWithOffset(1);
+ }
+
+ return Slices;
+}
+
+Optional<std::vector<StringRef>>
+SourceFromRanges(const std::vector<SourceRange> &Ranges,
+ const SourceManager &SM, const LangOptions &LangOpts) {
+ std::vector<StringRef> Snippets;
+ Snippets.reserve(Ranges.size());
+
+ for (const auto &Range : Ranges) {
+ CharSourceRange CR = Lexer::getAsCharRange(
+ CharSourceRange::getCharRange(Range.getBegin(), Range.getEnd()), SM,
+ LangOpts);
+
+ if (CR.isInvalid()) {
+#if PRINT_DEBUG
+ std::cerr << "CharSourceRange is invalid\n";
+#endif
+ return None;
+ }
+
+ bool InvalidText = false;
+ StringRef Snippet = Lexer::getSourceText(CR, SM, LangOpts, &InvalidText);
+
+ if (InvalidText) {
+#if PRINT_DEBUG
+ std::cerr << "SourcetText in invalid\n";
+#endif
+ return None;
+ }
+
+ Snippets.emplace_back(Snippet);
+ }
+
+ return Snippets;
+}
+
+/// Excepts a vector {TypeSnippet, Firstdecl, SecondDecl, ...}.
+std::vector<std::string>
+CreateIsolatedDecls(const std::vector<StringRef> &Snippets) {
+ // The first section is the type snippet, which does not make a decl itself.
+ assert(Snippets.size() >= 2 && "No enough snippets to create isolated decls");
+ std::vector<std::string> Decls(Snippets.size() - 1);
+
+ for (std::size_t i = 1, End = Snippets.size(); i < End; ++i)
+ Decls[i - 1] = Twine(Snippets[0].str()).concat(Snippets[i].ltrim()).str();
+
+ return Decls;
+}
+
+std::string &TrimRight(std::string &str) {
+ auto it1 = std::find_if(str.rbegin(), str.rend(), [](char ch) {
+ return !std::isspace<char>(ch, std::locale::classic());
+ });
+ str.erase(it1.base(), str.end());
+ return str;
+}
+
+std::string Concat(const std::vector<std::string> &Decls, StringRef Indent) {
+ std::string R;
+ for (const StringRef &D : Decls)
+ R += Twine(D).concat(";\n").concat(Indent).str();
+ return TrimRight(R);
+}
+
+} // namespace
+
+void IsolateDeclCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *WholeDecl = Result.Nodes.getNodeAs<DeclStmt>("decl_stmt");
+
+ auto Diag =
+ diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables")
+ << static_cast<unsigned int>(
+ std::distance(WholeDecl->decl_begin(), WholeDecl->decl_end()));
+
+ Optional<std::vector<SourceRange>> PotentialSlices =
+ DeclSlice(WholeDecl, *Result.SourceManager, getLangOpts());
+ if (!PotentialSlices)
+ return;
+
+ const std::vector<SourceRange> &Slices = *PotentialSlices;
+
+ Optional<std::vector<StringRef>> PotentialSnippets =
+ SourceFromRanges(Slices, *Result.SourceManager, getLangOpts());
+
+ if (!PotentialSnippets)
+ return;
+
+ std::vector<StringRef> Snippets = *PotentialSnippets;
+
+ std::vector<std::string> NewDecls = CreateIsolatedDecls(Snippets);
+ std::string Replacement =
+ Concat(NewDecls, Lexer::getIndentationForLine(WholeDecl->getBeginLoc(),
+ *Result.SourceManager));
+#if PRINT_DEBUG
+ std::cerr << Replacement << "\n";
+#endif
+
+ Diag << FixItHint::CreateReplacement(WholeDecl->getSourceRange(),
+ Replacement);
+}
+} // namespace readability
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/readability/CMakeLists.txt
===================================================================
--- clang-tidy/readability/CMakeLists.txt
+++ clang-tidy/readability/CMakeLists.txt
@@ -11,6 +11,7 @@
IdentifierNamingCheck.cpp
ImplicitBoolConversionCheck.cpp
InconsistentDeclarationParameterNameCheck.cpp
+ IsolateDeclCheck.cpp
MagicNumbersCheck.cpp
MisleadingIndentationCheck.cpp
MisplacedArrayIndexCheck.cpp
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
