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 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,281 @@ +//===--- 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 +#include + +using namespace clang::ast_matchers; + +#define PRINT_DEBUG 1 + +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(D) == nullptr; }); +} + +AST_MATCHER_P(IfStmt, hasInit, ast_matchers::internal::Matcher, + 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 +SourceLocation findNextAnyTokenKind(SourceLocation Start, + const SourceManager &SM, + const LangOptions &LangOpts, TokenKind TK, + TokenKinds... TKs) { + while (true) { + Optional 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> 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 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(*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(*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> +SourceFromRanges(const std::vector &Ranges, + const SourceManager &SM, const LangOptions &LangOpts) { + std::vector 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 +CreateIsolatedDecls(const std::vector &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 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(ch, std::locale::classic()); + }); + str.erase(it1.base(), str.end()); + return str; +} + +std::string Concat(const std::vector &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("decl_stmt"); + + /// TODO Include number of declared variables in diagnostic + auto Diag = diag(WholeDecl->getBeginLoc(), + "declaring multiple variables in one statement"); + + Optional> PotentialSlices = + DeclSlice(WholeDecl, *Result.SourceManager, getLangOpts()); + if (!PotentialSlices) + return; + + const std::vector &Slices = *PotentialSlices; + + Optional> PotentialSnippets = + SourceFromRanges(Slices, *Result.SourceManager, getLangOpts()); + + if (!PotentialSnippets) + return; + + std::vector Snippets = *PotentialSnippets; + + std::vector 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/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( "readability-inconsistent-declaration-parameter-name"); + CheckFactories.registerCheck( + "readability-isolate-decl"); CheckFactories.registerCheck( "readability-magic-numbers"); CheckFactories.registerCheck( 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 + ` check. + + FIXME: add release notes. + - New :doc:`readability-magic-numbers ` check. 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/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: 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 +struct pair { + T1 first; + T2 second; + pair(T1 v1, T2 v2) : first(v1), second(v2) {} + + template + 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(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: declaring multiple variables in one statement + // CHECK-FIXES: SomeClass v1; + // CHECK-FIXES: {{^ }}SomeClass v2(42); + // CHECK-FIXES: {{^ }}SomeClass v3{42}; + // CHECK-FIXES: {{^ }}SomeClass v4(42.5); +} Index: test/clang-tidy/readability-isolate-decl.cpp =================================================================== --- /dev/null +++ test/clang-tidy/readability-isolate-decl.cpp @@ -0,0 +1,112 @@ +// 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: declaring multiple variables in one 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: declaring multiple variables in one statement + // CHECK-FIXES: int normal; + // CHECK-FIXES: {{^ }}int weird = /* comment */ 42; +} + +void f3() { + int i, *pointer1; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: declaring multiple variables in one statement + // CHECK-FIXES: int i; + // CHECK-FIXES: {{^ }}int *pointer1; + // + int *pointer2 = nullptr, *pointer3 = &i; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: declaring multiple variables in one 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: declaring multiple variables in one 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: declaring multiple variables in one 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: declaring multiple variables in one 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: declaring multiple variables in one 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 +struct TemplatedType { + TemplatedType() = default; + TemplatedType(T value); +}; + +void f7() { + TemplatedType TT1(42), TT2{42}, TT3; + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: declaring multiple variables in one statement + // CHECK-FIXES: TemplatedType TT1(42); + // CHECK-FIXES: {{^ }}TemplatedType TT2{42}; + // CHECK-FIXES: {{^ }}TemplatedType 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: declaring multiple variables in one statement + // 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: declaring multiple variables in one statement + + int VAR_NAME(v3), + VAR_NAME(v4), + VAR_NAME(v5); + // CHECK-MESSAGES: [[@LINE-3]]:3: warning: declaring multiple variables in one statement + + A_BUNCH_OF_VARIABLES + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: declaring multiple variables in one statement +}