diff options
Diffstat (limited to 'clang-tools-extra/clang-tidy')
6 files changed, 440 insertions, 0 deletions
diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt index bd010af99a3..dace226aa6f 100644 --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -11,6 +11,7 @@ add_clang_library(clangTidyReadabilityModule IdentifierNamingCheck.cpp ImplicitBoolConversionCheck.cpp InconsistentDeclarationParameterNameCheck.cpp + IsolateDeclarationCheck.cpp MagicNumbersCheck.cpp MisleadingIndentationCheck.cpp MisplacedArrayIndexCheck.cpp diff --git a/clang-tools-extra/clang-tidy/readability/IsolateDeclarationCheck.cpp b/clang-tools-extra/clang-tidy/readability/IsolateDeclarationCheck.cpp new file mode 100644 index 00000000000..3155be39378 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/IsolateDeclarationCheck.cpp @@ -0,0 +1,279 @@ +//===--- IsolateDeclarationCheck.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 "IsolateDeclarationCheck.h" +#include "../utils/LexerUtils.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; +using namespace clang::tidy::utils::lexer; + +namespace clang { +namespace tidy { +namespace readability { + +namespace { +AST_MATCHER(DeclStmt, isSingleDecl) { return Node.isSingleDecl(); } +AST_MATCHER(DeclStmt, onlyDeclaresVariables) { + return llvm::all_of(Node.decls(), [](Decl *D) { return isa<VarDecl>(D); }); +} +} // namespace + +void IsolateDeclarationCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + declStmt(allOf(onlyDeclaresVariables(), unless(isSingleDecl()), + hasParent(compoundStmt()))) + .bind("decl_stmt"), + this); +} + +static SourceLocation findStartOfIndirection(SourceLocation Start, + int Indirections, + const SourceManager &SM, + const LangOptions &LangOpts) { + assert(Indirections >= 0 && "Indirections must be non-negative"); + if (Indirections == 0) + return Start; + + // Note that the post-fix decrement is necessary to perform the correct + // number of transformations. + while (Indirections-- != 0) { + Start = findPreviousAnyTokenKind(Start, SM, LangOpts, tok::star, tok::amp); + if (Start.isInvalid() || Start.isMacroID()) + return SourceLocation(); + } + return Start; +} + +static bool isMacroID(SourceRange R) { + return R.getBegin().isMacroID() || R.getEnd().isMacroID(); +} + +/// This function counts the number of written indirections for the given +/// Type \p T. It does \b NOT resolve typedefs as it's a helper for lexing +/// the source code. +/// \see declRanges +static int countIndirections(const Type *T, int Indirections = 0) { + if (T->isFunctionPointerType()) { + const auto *Pointee = T->getPointeeType()->castAs<FunctionType>(); + return countIndirections( + Pointee->getReturnType().IgnoreParens().getTypePtr(), ++Indirections); + } + + // Note: Do not increment the 'Indirections' because it is not yet clear + // if there is an indirection added in the source code of the array + // declaration. + if (const auto *AT = dyn_cast<ArrayType>(T)) + return countIndirections(AT->getElementType().IgnoreParens().getTypePtr(), + Indirections); + + if (isa<PointerType>(T) || isa<ReferenceType>(T)) + return countIndirections(T->getPointeeType().IgnoreParens().getTypePtr(), + ++Indirections); + + return Indirections; +} + +static bool typeIsMemberPointer(const Type *T) { + if (isa<ArrayType>(T)) + return typeIsMemberPointer(T->getArrayElementTypeNoTypeQual()); + + if ((isa<PointerType>(T) || isa<ReferenceType>(T)) && + isa<PointerType>(T->getPointeeType())) + return typeIsMemberPointer(T->getPointeeType().getTypePtr()); + + return isa<MemberPointerType>(T); +} + +/// This function tries to extract the SourceRanges that make up all +/// declarations in this \c DeclStmt. +/// +/// The resulting vector has the structure {UnderlyingType, Decl1, Decl2, ...}. +/// Each \c SourceRange is of the form [Begin, End). +/// If any of the create ranges is invalid or in a macro the result will be +/// \c None. +/// If the \c DeclStmt contains only one declaration, the result is \c None. +/// If the \c DeclStmt contains declarations other than \c VarDecl the result +/// is \c None. +/// +/// \code +/// int * ptr1 = nullptr, value = 42; +/// // [ ][ ] [ ] - The ranges here are inclusive +/// \endcode +/// \todo Generalize this function to take other declarations than \c VarDecl. +static Optional<std::vector<SourceRange>> +declRanges(const DeclStmt *DS, const SourceManager &SM, + const LangOptions &LangOpts) { + std::size_t DeclCount = std::distance(DS->decl_begin(), DS->decl_end()); + if (DeclCount < 2) + return None; + + if (rangeContainsExpansionsOrDirectives(DS->getSourceRange(), SM, LangOpts)) + return None; + + // The initial type of the declaration and each declaration has it's own + // slice. This is necessary, because pointers and references bind only + // to the local variable and not to all variables in the declaration. + // Example: 'int *pointer, value = 42;' + std::vector<SourceRange> Slices; + Slices.reserve(DeclCount + 1); + + // Calculate the first slice, for now only variables are handled but in the + // future this should be relaxed and support various kinds of declarations. + const auto *FirstDecl = dyn_cast<VarDecl>(*DS->decl_begin()); + + if (FirstDecl == nullptr) + return None; + + // FIXME: Member pointers are not transformed correctly right now, that's + // why they are treated as problematic here. + if (typeIsMemberPointer(FirstDecl->getType().IgnoreParens().getTypePtr())) + return None; + + // Consider the following case: 'int * pointer, value = 42;' + // Created slices (inclusive) [ ][ ] [ ] + // Because 'getBeginLoc' points to the start of the variable *name*, the + // location of the pointer must be determined separatly. + SourceLocation Start = findStartOfIndirection( + FirstDecl->getLocation(), + countIndirections(FirstDecl->getType().IgnoreParens().getTypePtr()), SM, + LangOpts); + + // Fix function-pointer declarations that have a '(' in front of the + // pointer. + // Example: 'void (*f2)(int), (*g2)(int, float) = gg;' + // Slices: [ ][ ] [ ] + if (FirstDecl->getType()->isFunctionPointerType()) + Start = findPreviousTokenKind(Start, SM, LangOpts, tok::l_paren); + + // It is popssible that a declarator is wrapped with parens. + // Example: 'float (((*f_ptr2)))[42], *f_ptr3, ((f_value2)) = 42.f;' + // The slice for the type-part must not contain these parens. Consequently + // 'Start' is moved to the most left paren if there are parens. + while (true) { + if (Start.isInvalid() || Start.isMacroID()) + break; + + Token T = getPreviousToken(Start, SM, LangOpts); + if (T.is(tok::l_paren)) { + Start = findPreviousTokenStart(Start, SM, LangOpts); + continue; + } + break; + } + + SourceRange DeclRange(DS->getBeginLoc(), Start); + if (DeclRange.isInvalid() || isMacroID(DeclRange)) + return None; + + // The first slice, that is prepended to every isolated declaration, is + // created. + Slices.emplace_back(DeclRange); + + // Create all following slices that each declare a variable. + SourceLocation DeclBegin = Start; + for (const auto &Decl : DS->decls()) { + const auto *CurrentDecl = cast<VarDecl>(Decl); + + // FIXME: Member pointers are not transformed correctly right now, that's + // why they are treated as problematic here. + if (typeIsMemberPointer(CurrentDecl->getType().IgnoreParens().getTypePtr())) + return None; + + SourceLocation DeclEnd = + CurrentDecl->hasInit() + ? findNextTerminator(CurrentDecl->getInit()->getEndLoc(), SM, + LangOpts) + : findNextTerminator(CurrentDecl->getEndLoc(), SM, LangOpts); + + SourceRange VarNameRange(DeclBegin, DeclEnd); + if (VarNameRange.isInvalid() || isMacroID(VarNameRange)) + return None; + + Slices.emplace_back(VarNameRange); + DeclBegin = DeclEnd.getLocWithOffset(1); + } + return Slices; +} + +static Optional<std::vector<StringRef>> +collectSourceRanges(llvm::ArrayRef<SourceRange> Ranges, const SourceManager &SM, + const LangOptions &LangOpts) { + std::vector<StringRef> Snippets; + Snippets.reserve(Ranges.size()); + + for (const auto &Range : Ranges) { + CharSourceRange CharRange = Lexer::getAsCharRange( + CharSourceRange::getCharRange(Range.getBegin(), Range.getEnd()), SM, + LangOpts); + + if (CharRange.isInvalid()) + return None; + + bool InvalidText = false; + StringRef Snippet = + Lexer::getSourceText(CharRange, SM, LangOpts, &InvalidText); + + if (InvalidText) + return None; + + Snippets.emplace_back(Snippet); + } + + return Snippets; +} + +/// Expects a vector {TypeSnippet, Firstdecl, SecondDecl, ...}. +static std::vector<std::string> +createIsolatedDecls(llvm::ArrayRef<StringRef> Snippets) { + // The first section is the type snippet, which does not make a decl itself. + assert(Snippets.size() > 2 && "Not enough snippets to create isolated decls"); + std::vector<std::string> Decls(Snippets.size() - 1); + + for (std::size_t I = 1; I < Snippets.size(); ++I) + Decls[I - 1] = Twine(Snippets[0]) + .concat(Snippets[0].endswith(" ") ? "" : " ") + .concat(Snippets[I].ltrim()) + .concat(";") + .str(); + + return Decls; +} + +void IsolateDeclarationCheck::check(const MatchFinder::MatchResult &Result) { + const auto *WholeDecl = Result.Nodes.getNodeAs<DeclStmt>("decl_stmt"); + + auto Diag = + diag(WholeDecl->getBeginLoc(), + "multiple declarations in a single statement reduces readability"); + + Optional<std::vector<SourceRange>> PotentialRanges = + declRanges(WholeDecl, *Result.SourceManager, getLangOpts()); + if (!PotentialRanges) + return; + + Optional<std::vector<StringRef>> PotentialSnippets = collectSourceRanges( + *PotentialRanges, *Result.SourceManager, getLangOpts()); + + if (!PotentialSnippets) + return; + + std::vector<std::string> NewDecls = createIsolatedDecls(*PotentialSnippets); + std::string Replacement = llvm::join( + NewDecls, + (Twine("\n") + Lexer::getIndentationForLine(WholeDecl->getBeginLoc(), + *Result.SourceManager)) + .str()); + + Diag << FixItHint::CreateReplacement(WholeDecl->getSourceRange(), + Replacement); +} +} // namespace readability +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/readability/IsolateDeclarationCheck.h b/clang-tools-extra/clang-tidy/readability/IsolateDeclarationCheck.h new file mode 100644 index 00000000000..b7f4793d9d1 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/IsolateDeclarationCheck.h @@ -0,0 +1,36 @@ +//===--- IsolateDeclarationCheck.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 { + +/// This check diagnoses all DeclStmt's declaring more than one variable and +/// tries to refactor the code to one statement per declaration. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability-isolate-declaration.html +class IsolateDeclarationCheck : public ClangTidyCheck { +public: + IsolateDeclarationCheck(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 diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp index ca49f6f118c..5855bb07485 100644 --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -20,6 +20,7 @@ #include "IdentifierNamingCheck.h" #include "ImplicitBoolConversionCheck.h" #include "InconsistentDeclarationParameterNameCheck.h" +#include "IsolateDeclarationCheck.h" #include "MagicNumbersCheck.h" #include "MisleadingIndentationCheck.h" #include "MisplacedArrayIndexCheck.h" @@ -67,6 +68,8 @@ public: "readability-implicit-bool-conversion"); CheckFactories.registerCheck<InconsistentDeclarationParameterNameCheck>( "readability-inconsistent-declaration-parameter-name"); + CheckFactories.registerCheck<IsolateDeclarationCheck>( + "readability-isolate-declaration"); CheckFactories.registerCheck<MagicNumbersCheck>( "readability-magic-numbers"); CheckFactories.registerCheck<MisleadingIndentationCheck>( diff --git a/clang-tools-extra/clang-tidy/utils/LexerUtils.cpp b/clang-tools-extra/clang-tidy/utils/LexerUtils.cpp index 1b52347e45d..82097978129 100644 --- a/clang-tools-extra/clang-tidy/utils/LexerUtils.cpp +++ b/clang-tools-extra/clang-tidy/utils/LexerUtils.cpp @@ -31,6 +31,67 @@ Token getPreviousToken(SourceLocation Location, const SourceManager &SM, return Token; } +SourceLocation findPreviousTokenStart(SourceLocation Start, + const SourceManager &SM, + const LangOptions &LangOpts) { + if (Start.isInvalid() || Start.isMacroID()) + return SourceLocation(); + + SourceLocation BeforeStart = Start.getLocWithOffset(-1); + if (BeforeStart.isInvalid() || BeforeStart.isMacroID()) + return SourceLocation(); + + return Lexer::GetBeginningOfToken(BeforeStart, SM, LangOpts); +} + +SourceLocation findPreviousTokenKind(SourceLocation Start, + const SourceManager &SM, + const LangOptions &LangOpts, + tok::TokenKind TK) { + while (true) { + SourceLocation L = findPreviousTokenStart(Start, SM, LangOpts); + if (L.isInvalid() || L.isMacroID()) + return SourceLocation(); + + Token T; + if (Lexer::getRawToken(L, T, SM, LangOpts, /*IgnoreWhiteSpace=*/true)) + return SourceLocation(); + + if (T.is(TK)) + return T.getLocation(); + + Start = L; + } +} + +SourceLocation findNextTerminator(SourceLocation Start, const SourceManager &SM, + const LangOptions &LangOpts) { + return findNextAnyTokenKind(Start, SM, LangOpts, tok::comma, tok::semi); +} + +bool rangeContainsExpansionsOrDirectives(SourceRange Range, + const SourceManager &SM, + const LangOptions &LangOpts) { + assert(Range.isValid() && "Invalid Range for relexing provided"); + SourceLocation Loc = Range.getBegin(); + + while (Loc < Range.getEnd()) { + if (Loc.isMacroID()) + return true; + + llvm::Optional<Token> Tok = Lexer::findNextToken(Loc, SM, LangOpts); + + if (!Tok) + return true; + + if (Tok->is(tok::hash)) + return true; + + Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, LangOpts).getLocWithOffset(1); + } + + return false; +} } // namespace lexer } // namespace utils } // namespace tidy diff --git a/clang-tools-extra/clang-tidy/utils/LexerUtils.h b/clang-tools-extra/clang-tidy/utils/LexerUtils.h index 0eb5e56ef64..f81fdae4b52 100644 --- a/clang-tools-extra/clang-tidy/utils/LexerUtils.h +++ b/clang-tools-extra/clang-tidy/utils/LexerUtils.h @@ -22,6 +22,66 @@ namespace lexer { Token getPreviousToken(SourceLocation Location, const SourceManager &SM, const LangOptions &LangOpts, bool SkipComments = true); +SourceLocation findPreviousTokenStart(SourceLocation Start, + const SourceManager &SM, + const LangOptions &LangOpts); + +SourceLocation findPreviousTokenKind(SourceLocation Start, + const SourceManager &SM, + const LangOptions &LangOpts, + tok::TokenKind TK); + +SourceLocation findNextTerminator(SourceLocation Start, const SourceManager &SM, + const LangOptions &LangOpts); + +template <typename TokenKind, typename... TokenKinds> +SourceLocation findPreviousAnyTokenKind(SourceLocation Start, + const SourceManager &SM, + const LangOptions &LangOpts, + TokenKind TK, TokenKinds... TKs) { + while (true) { + SourceLocation L = findPreviousTokenStart(Start, SM, LangOpts); + if (L.isInvalid() || L.isMacroID()) + return SourceLocation(); + + Token T; + // Returning 'true' is used to signal failure to retrieve the token. + if (Lexer::getRawToken(L, T, SM, LangOpts)) + return SourceLocation(); + + if (T.isOneOf(TK, TKs...)) + return T.getLocation(); + + Start = L; + } +} + +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) + return SourceLocation(); + + Token PotentialMatch = *CurrentToken; + if (PotentialMatch.isOneOf(TK, TKs...)) + return PotentialMatch.getLocation(); + + Start = PotentialMatch.getLastLoc(); + } +} + +/// Re-lex the provide \p Range and return \c false if either a macro spans +/// multiple tokens, a pre-processor directive or failure to retrieve the +/// next token is found, otherwise \c true. +bool rangeContainsExpansionsOrDirectives(SourceRange Range, + const SourceManager &SM, + const LangOptions &LangOpts); + } // namespace lexer } // namespace utils } // namespace tidy |
