JonasToth updated this revision to Diff 165321.
JonasToth added a comment.
This is the newer and better working code to slice and separate multiple
vardecls in a multi-decl stmt.
I want this version only for now, as the multiple variable definitions in one
statement are by far the most common variant of the single-decl-per-stmt rule.
All other cases are supposed to be diagnosed only.
The next steps will be:
- add the testsuite of the other abondended check
- dont fixit for places like `for(int start = 0, end = 42; ...)`
This patch includes:
- do proper transformations for normal vardecls
- finalize current tests and trim right whitespace of last generated replacement
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.cpp
Index: test/clang-tidy/readability-isolate-decl.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/readability-isolate-decl.cpp
@@ -0,0 +1,82 @@
+// 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: make only one declaration per statement
+ // 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: make only one declaration per statement
+ // CHECK-FIXES: int normal;
+ // CHECK-FIXES: {{^ }}int weird = /* comment */ 42;
+}
+
+void f3() {
+ int i, *pointer1;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: make only one declaration per statement
+ // CHECK-FIXES: int i;
+ // CHECK-FIXES: {{^ }}int *pointer1;
+ //
+ int *pointer2 = nullptr, *pointer3 = &i;
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: make only one declaration per statement
+ // 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: make only one declaration per statement
+ // 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: make only one declaration per statement
+ // 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: make only one declaration per statement
+ // 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: make only one declaration per statement
+ // 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: make only one declaration per statement
+ // CHECK-FIXES: TemplatedType<int> TT1(42);
+ // CHECK-FIXES: {{^ }}TemplatedType<int> TT2{42};
+ // CHECK-FIXES: {{^ }}TemplatedType<int> TT3;
+}
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,205 @@
+//===--- 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 <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(); }
+} // namespace
+
+void IsolateDeclCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(declStmt(unless(isSingleDecl())).bind("decl_stmt"), this);
+}
+
+namespace {
+SourceLocation findNextTerminator(SourceLocation Start, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ while (true) {
+ Optional<Token> CurrentToken = Lexer::findNextToken(Start, SM, LangOpts);
+ assert(CurrentToken.hasValue() && "Could not find token in Decl");
+ Token PotentialComma = *CurrentToken;
+
+ if (PotentialComma.getKind() == tok::comma ||
+ PotentialComma.getKind() == tok::semi)
+ return PotentialComma.getLocation();
+
+ Start =
+ Lexer::getLocForEndOfToken(Start, 0, SM, LangOpts).getLocWithOffset(1);
+ }
+}
+
+/// This function goes back in the source text, to find the beginning of
+/// indirection.
+/// Necessary for `int *p, i;` where `p` has one level of indirection.
+/// To isolate the decl properly, the first Slice must only be `int` and not
+/// `int *`.
+SourceLocation findStartOfIndirection(SourceLocation Start,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ while (true) {
+ Optional<Token> CurrentToken = Lexer::findNextToken(Start, SM, LangOpts);
+ assert(CurrentToken.hasValue() && "Could not find token in Decl");
+ Token PotentialIndirection = *CurrentToken;
+
+ if (PotentialIndirection.getKind() == tok::star ||
+ PotentialIndirection.getKind() == tok::amp)
+ return PotentialIndirection.getLocation();
+
+ Start =
+ Lexer::getLocForEndOfToken(Start, 0, SM, LangOpts).getLocWithOffset(1);
+ }
+}
+
+/// This function assumes, that \p DS only contains VarDecl elements.
+std::vector<SourceRange> DeclSlice(const DeclStmt *DS, const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ // The initial type of the declaration and each declaration has it's slice.
+ std::vector<SourceRange> Slices;
+ Slices.reserve(std::distance(DS->decl_begin(), DS->decl_end()) + 1);
+#if PRINT_DEBUG
+ std::cerr << "Number of Slices to create: " << Slices.capacity() << "\n";
+#endif
+ assert(Slices.capacity() > 2 &&
+ "Expect at least 3 sections (type, first decl, second decl) ");
+
+ // 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
+ Slices.emplace_back(SourceRange(DS->getBeginLoc(), Start));
+
+ 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
+ Slices.emplace_back(SourceRange(DeclBegin, DeclEnd));
+ DeclBegin = DeclEnd.getLocWithOffset(1);
+ }
+
+ return Slices;
+}
+
+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);
+ assert(CR.isValid() && "Retrieved invalid char source range");
+
+ bool InvalidText = false;
+ StringRef Snippet = Lexer::getSourceText(CR, SM, LangOpts, &InvalidText);
+ assert(!InvalidText && "Retrieved invalid text");
+
+ 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] = Snippets[0].str() + 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 std::string &D : Decls)
+ R += D + ";\n" + Indent.str();
+ return TrimRight(R);
+}
+
+} // namespace
+
+void IsolateDeclCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *WholeDecl = Result.Nodes.getNodeAs<DeclStmt>("decl_stmt");
+
+ std::vector<SourceRange> Slices =
+ DeclSlice(WholeDecl, *Result.SourceManager, getLangOpts());
+#if PRINT_DEBUG
+ std::cerr << "Number of Slices: " << Slices.size() << "\n";
+ for (const auto &Slice : Slices) {
+ diag(Slice.getBegin(), "Slice start", DiagnosticIDs::Note);
+ diag(Slice.getEnd(), "Slice End", DiagnosticIDs::Note);
+ }
+#endif
+
+ std::vector<StringRef> Snippets =
+ SourceFromRanges(Slices, *Result.SourceManager, getLangOpts());
+
+ std::vector<std::string> NewDecls = CreateIsolatedDecls(Snippets);
+ std::string Replacement =
+ Concat(NewDecls, Lexer::getIndentationForLine(WholeDecl->getBeginLoc(),
+ *Result.SourceManager));
+
+ std::cerr << Replacement << "\n";
+
+ diag(WholeDecl->getBeginLoc(), "make only one declaration per statement")
+ << 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
