From d4637fb433bdef381dbe2f6a973d3a33e42e5907 Mon Sep 17 00:00:00 2001 From: Gabor Horvath Date: Tue, 10 Feb 2015 09:14:26 +0000 Subject: [PATCH] [clang-tidy] Checker for inaccurate use of erase() method. Algorithms like remove() does not actually remove any element from the container but returns an iterator to the first redundant element at the end of the container. These redundant elements must be removed using the erase() method. This check warns when not all of the elements will be removed due to using an inappropriate overload. Reviewer: alexfh Differential Revision: http://reviews.llvm.org/D7496 llvm-svn: 228679 --- .../clang-tidy/misc/CMakeLists.txt | 1 + .../clang-tidy/misc/InaccurateEraseCheck.cpp | 64 ++++++++++++++++++ .../clang-tidy/misc/InaccurateEraseCheck.h | 36 ++++++++++ .../clang-tidy/misc/MiscTidyModule.cpp | 3 + .../test/clang-tidy/misc-inaccurate-erase.cpp | 67 +++++++++++++++++++ 5 files changed, 171 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/misc/InaccurateEraseCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/misc/InaccurateEraseCheck.h create mode 100644 clang-tools-extra/test/clang-tidy/misc-inaccurate-erase.cpp diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index b68648b629ee..1f2d9960a502 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -4,6 +4,7 @@ add_clang_library(clangTidyMiscModule ArgumentCommentCheck.cpp AssignOperatorSignatureCheck.cpp BoolPointerImplicitConversion.cpp + InaccurateEraseCheck.cpp InefficientAlgorithmCheck.cpp MiscTidyModule.cpp SwappedArgumentsCheck.cpp diff --git a/clang-tools-extra/clang-tidy/misc/InaccurateEraseCheck.cpp b/clang-tools-extra/clang-tidy/misc/InaccurateEraseCheck.cpp new file mode 100644 index 000000000000..033e0a15d876 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/InaccurateEraseCheck.cpp @@ -0,0 +1,64 @@ +//===--- InaccurateEraseCheck.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 "InaccurateEraseCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { + +void InaccurateEraseCheck::registerMatchers(MatchFinder *Finder) { + const auto CheckForEndCall = hasArgument( + 1, + anyOf(constructExpr(has(memberCallExpr(callee(methodDecl(hasName("end")))) + .bind("InaccEndCall"))), + anything())); + + Finder->addMatcher( + memberCallExpr( + on(hasType(namedDecl(matchesName("std::")))), + callee(methodDecl(hasName("erase"))), argumentCountIs(1), + hasArgument(0, has(callExpr(callee(functionDecl(matchesName( + "std::(remove_if|remove|unique)"))), + CheckForEndCall).bind("InaccAlgCall"))), + unless(isInTemplateInstantiation())).bind("InaccErase"), + this); +} + +void InaccurateEraseCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MemberCall = + Result.Nodes.getNodeAs("InaccErase"); + const auto *EndExpr = + Result.Nodes.getNodeAs("InaccEndCall"); + const SourceLocation Loc = MemberCall->getLocStart(); + + FixItHint Hint; + + if (!Loc.isMacroID() && EndExpr) { + const auto *AlgCall = Result.Nodes.getNodeAs("InaccAlgCall"); + std::string ReplacementText = Lexer::getSourceText( + CharSourceRange::getTokenRange(EndExpr->getSourceRange()), + *Result.SourceManager, Result.Context->getLangOpts()); + const SourceLocation EndLoc = Lexer::getLocForEndOfToken( + AlgCall->getLocEnd(), 0, *Result.SourceManager, + Result.Context->getLangOpts()); + Hint = FixItHint::CreateInsertion(EndLoc, ", " + ReplacementText); + } + + diag(Loc, "this call will remove at most one item even when multiple items " + "should be removed") + << Hint; +} + +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/misc/InaccurateEraseCheck.h b/clang-tools-extra/clang-tidy/misc/InaccurateEraseCheck.h new file mode 100644 index 000000000000..cde7320a2be9 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/InaccurateEraseCheck.h @@ -0,0 +1,36 @@ +//===--- InaccurateEraseCheck.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_MISC_INACCURATE_ERASE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INACCURATE_ERASE_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { + +/// \brief Checks for inaccurate use of \c erase() method. +/// +/// Algorithms like \c remove() do not actually remove any element from the +/// container but return an iterator to the first redundant element at the end +/// of the container. These redundant elements must be removed using the +/// \c erase() method. This check warns when not all of the elements will be +/// removed due to using an inappropriate overload. +class InaccurateEraseCheck : public ClangTidyCheck { +public: + InaccurateEraseCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_INACCURATE_ERASE_H diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index dfd5a49986b5..a43efabcfd34 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -13,6 +13,7 @@ #include "ArgumentCommentCheck.h" #include "AssignOperatorSignatureCheck.h" #include "BoolPointerImplicitConversion.h" +#include "InaccurateEraseCheck.h" #include "InefficientAlgorithmCheck.h" #include "SwappedArgumentsCheck.h" #include "UndelegatedConstructor.h" @@ -31,6 +32,8 @@ public: "misc-assign-operator-signature"); CheckFactories.registerCheck( "misc-bool-pointer-implicit-conversion"); + CheckFactories.registerCheck( + "misc-inaccurate-erase"); CheckFactories.registerCheck( "misc-inefficient-algorithm"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/test/clang-tidy/misc-inaccurate-erase.cpp b/clang-tools-extra/test/clang-tidy/misc-inaccurate-erase.cpp new file mode 100644 index 000000000000..1693b9f455c4 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/misc-inaccurate-erase.cpp @@ -0,0 +1,67 @@ +// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-inaccurate-erase %t +// REQUIRES: shell + +namespace std { +struct vec_iterator {}; + +template struct vector { + typedef vec_iterator iterator; + + iterator begin(); + iterator end(); + + void erase(iterator); + void erase(iterator, iterator); +}; + +template +FwIt remove(FwIt begin, FwIt end, const T &val); + +template +FwIt remove_if(FwIt begin, FwIt end, Func f); + +template FwIt unique(FwIt begin, FwIt end); +} // namespace std + +struct custom_iter {}; +struct custom_container { + void erase(...); + custom_iter begin(); + custom_iter end(); +}; + +template void g() { + T t; + t.erase(std::remove(t.begin(), t.end(), 10)); + // CHECK-FIXES: {{^ }}t.erase(std::remove(t.begin(), t.end(), 10));{{$}} + + std::vector v; + v.erase(remove(v.begin(), v.end(), 10)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this call will remove at most one + // CHECK-FIXES: {{^ }}v.erase(remove(v.begin(), v.end(), 10), v.end());{{$}} +} + +#define ERASE(x, y) x.erase(remove(x.begin(), x.end(), y)) +// CHECK-FIXES: #define ERASE(x, y) x.erase(remove(x.begin(), x.end(), y)) + +int main() { + std::vector v; + + v.erase(remove(v.begin(), v.end(), 10)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this call will remove at most one item even when multiple items should be removed [misc-inaccurate-erase] + // CHECK-FIXES: {{^ }}v.erase(remove(v.begin(), v.end(), 10), v.end());{{$}} + v.erase(remove(v.begin(), v.end(), 20), v.end()); + + // Fix is not trivial. + auto it = v.end(); + v.erase(remove(v.begin(), it, 10)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this call will remove at most one + // CHECK-FIXES: {{^ }}v.erase(remove(v.begin(), it, 10));{{$}} + + g>(); + g(); + + ERASE(v, 15); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: this call will remove at most one + // CHECK-FIXES: {{^ }}ERASE(v, 15);{{$}} +}