Index: clang-tidy/readability/CMakeLists.txt =================================================================== --- clang-tidy/readability/CMakeLists.txt +++ clang-tidy/readability/CMakeLists.txt @@ -2,6 +2,7 @@ add_clang_library(clangTidyReadabilityModule BracesAroundStatementsCheck.cpp + ContainerSizeEmpty.cpp ElseAfterReturnCheck.cpp FunctionSize.cpp NamespaceCommentCheck.cpp Index: clang-tidy/readability/ContainerSizeEmpty.h =================================================================== --- clang-tidy/readability/ContainerSizeEmpty.h +++ clang-tidy/readability/ContainerSizeEmpty.h @@ -0,0 +1,40 @@ +//===--- ContainerSizeEmpty.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_CONTAINERSIZEEMPTY_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONTAINERSIZEEMPTY_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace readability { + +/// \brief Checks whether a call to the \c size() method can be replaced with a +/// call to \c empty(). +/// +/// The emptiness of a container should be checked using the \c empty() method +/// instead of the \c size() method. It is not guaranteed that \c size() is a +/// constant-time function, and it is generally more efficient and also shows +/// clearer intent to use \c empty(). Furthermore some containers may implement +/// the \c empty() method but not implement the \c size() method. Using \c +/// empty() whenever possible makes it easier to switch to another container in +/// the future. +class ContainerSizeEmptyCheck : public ClangTidyCheck { +public: + ContainerSizeEmptyCheck(StringRef Name, ClangTidyContext *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_CONTAINERSIZEEMPTY_H Index: clang-tidy/readability/ContainerSizeEmpty.cpp =================================================================== --- clang-tidy/readability/ContainerSizeEmpty.cpp +++ clang-tidy/readability/ContainerSizeEmpty.cpp @@ -0,0 +1,173 @@ +//===--- ContainerSizeEmpty.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 "ContainerSizeEmpty.h" + +#include "llvm/ADT/StringRef.h" +#include "llvm/ADT/StringSet.h" + +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/ASTMatchers/ASTMatchersInternal.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace { +bool isContainer(llvm::StringRef ClassName) { + static const llvm::StringSet<> ContainerNames = [] { + llvm::StringSet<> RetVal; + RetVal.insert("std::vector"); + RetVal.insert("std::list"); + RetVal.insert("std::array"); + RetVal.insert("std::deque"); + RetVal.insert("std::forward_list"); + RetVal.insert("std::set"); + RetVal.insert("std::map"); + RetVal.insert("std::multiset"); + RetVal.insert("std::multimap"); + RetVal.insert("std::unordered_set"); + RetVal.insert("std::unordered_map"); + RetVal.insert("std::unordered_multiset"); + RetVal.insert("std::unordered_multimap"); + RetVal.insert("std::stack"); + RetVal.insert("std::queue"); + RetVal.insert("std::priority_queue"); + return RetVal; + }(); + return ContainerNames.find(ClassName) != ContainerNames.end(); +} +} + +namespace clang { +namespace ast_matchers { +AST_MATCHER_P(QualType, unqualifiedType, internal::Matcher, + InnerMatcher) { + return InnerMatcher.matches(*Node, Finder, Builder); +} + +AST_MATCHER(Type, isBoolType) { return Node.isBooleanType(); } + +AST_MATCHER(NamedDecl, stlContainer) { + return isContainer(Node.getQualifiedNameAsString()); +} +} +} + +namespace clang { +namespace tidy { +namespace readability { + +ContainerSizeEmptyCheck::ContainerSizeEmptyCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + +void ContainerSizeEmptyCheck::registerMatchers(MatchFinder *Finder) { + const auto WrongUse = anyOf( + hasParent( + binaryOperator( + anyOf(has(integerLiteral(equals(0))), + allOf(anyOf(hasOperatorName("<"), hasOperatorName(">="), + hasOperatorName(">"), hasOperatorName("<=")), + anyOf(hasRHS(integerLiteral(equals(1))), + hasLHS(integerLiteral(equals(1))))))) + .bind("SizeBinaryOp")), + hasParent(implicitCastExpr( + hasImplicitDestinationType(unqualifiedType(isBoolType())), + anyOf( + hasParent(unaryOperator(hasOperatorName("!")).bind("NegOnSize")), + anything()))), + hasParent( + explicitCastExpr(hasDestinationType(unqualifiedType(isBoolType()))))); + + Finder->addMatcher( + memberCallExpr( + on(expr(anyOf(hasType(namedDecl(stlContainer())), + hasType(qualType(pointsTo(namedDecl(stlContainer())))), + hasType(qualType(references( + namedDecl(stlContainer())))))).bind("STLObject")), + callee(methodDecl(hasName("size"))), WrongUse).bind("SizeCallExpr"), + this); +} + +void ContainerSizeEmptyCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MemberCall = + Result.Nodes.getNodeAs("SizeCallExpr"); + const auto *BinaryOp = Result.Nodes.getNodeAs("SizeBinaryOp"); + const auto *E = Result.Nodes.getNodeAs("STLObject"); + FixItHint Hint; + std::string ReplacementText = + Lexer::getSourceText(CharSourceRange::getTokenRange(E->getSourceRange()), + *Result.SourceManager, LangOptions()); + if (E->getType()->isPointerType()) + ReplacementText += "->empty()"; + else + ReplacementText += ".empty()"; + + if (BinaryOp) { // Determine the correct transformation. + bool Negation = false; + const bool ContainerIsLHS = !llvm::isa(BinaryOp->getLHS()); + const auto OpCode = BinaryOp->getOpcode(); + uint64_t Value = 0; + if (ContainerIsLHS) { + if (const auto *Literal = + llvm::dyn_cast(BinaryOp->getRHS())) + Value = Literal->getValue().getLimitedValue(); + else + return; + } else { + Value = llvm::dyn_cast(BinaryOp->getLHS()) + ->getValue() + .getLimitedValue(); + } + + // Constant that is not handled. + if (Value > 1) + return; + + // Always true, no warnings for that. + if ((OpCode == BinaryOperatorKind::BO_GE && Value == 0 && ContainerIsLHS) || + (OpCode == BinaryOperatorKind::BO_LE && Value == 0 && !ContainerIsLHS)) + return; + + if (OpCode == BinaryOperatorKind::BO_NE && Value == 0) + Negation = true; + if ((OpCode == BinaryOperatorKind::BO_GT || + OpCode == BinaryOperatorKind::BO_GE) && + ContainerIsLHS) + Negation = true; + if ((OpCode == BinaryOperatorKind::BO_LT || + OpCode == BinaryOperatorKind::BO_LE) && + !ContainerIsLHS) + Negation = true; + + if (Negation) + ReplacementText = "!" + ReplacementText; + Hint = FixItHint::CreateReplacement(BinaryOp->getSourceRange(), + ReplacementText); + + } else { + // If there is a conversion above the size call to bool, it is safe to just + // replace size with empty. + if (const auto *UnaryOp = + Result.Nodes.getNodeAs("NegOnSize")) + Hint = FixItHint::CreateReplacement(UnaryOp->getSourceRange(), + ReplacementText); + else + Hint = FixItHint::CreateReplacement(MemberCall->getSourceRange(), + "!" + ReplacementText); + } + diag(MemberCall->getLocStart(), + "The 'empty' method should be used to check for emptiness instead " + "of 'size'.") + << Hint; +} + +} // namespace readability +} // namespace tidy +} // namespace clang Index: clang-tidy/readability/ReadabilityTidyModule.cpp =================================================================== --- clang-tidy/readability/ReadabilityTidyModule.cpp +++ clang-tidy/readability/ReadabilityTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" #include "BracesAroundStatementsCheck.h" +#include "ContainerSizeEmpty.h" #include "ElseAfterReturnCheck.h" #include "FunctionSize.h" #include "RedundantSmartptrGet.h" @@ -24,6 +25,8 @@ void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck( "readability-braces-around-statements"); + CheckFactories.registerCheck( + "readability-container-size-empty"); CheckFactories.registerCheck( "readability-else-after-return"); CheckFactories.registerCheck( Index: test/clang-tidy/readibility-container-size-empty.cpp =================================================================== --- test/clang-tidy/readibility-container-size-empty.cpp +++ test/clang-tidy/readibility-container-size-empty.cpp @@ -0,0 +1,106 @@ +// RUN: $(dirname %s)/check_clang_tidy.sh %s readability-container-size-empty %t +// REQUIRES: shell + +namespace std { +template struct vector { + vector() {} + int size() const {} + bool empty() const {} +}; +} + +int main() { + std::vector vect; + if (vect.size() == 0) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty] + // CHECK-FIXES: {{^ }}if (vect.empty()){{$}} + if (vect.size() != 0) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}} + if (0 == vect.size()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (vect.empty()){{$}} + if (0 != vect.size()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}} + if (vect.size() > 0) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}} + if (0 < vect.size()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}} + if (vect.size() < 1) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (vect.empty()){{$}} + if (1 > vect.size()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:11: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (vect.empty()){{$}} + if (vect.size() >= 1) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}} + if (1 <= vect.size()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:12: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}} + if (!vect.size()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (vect.empty()){{$}} + if (vect.size()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (!vect.empty()){{$}} + + if (vect.empty()) + ; + + const std::vector vect2; + if (vect2.size() != 0) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (!vect2.empty()){{$}} + + std::vector *vect3 = new std::vector(); + if (vect3->size() == 0) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (vect3->empty()){{$}} + + delete vect3; + + const std::vector &vect4 = vect2; + if (vect4.size() == 0) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (vect4.empty()){{$}} +} + +#define CHECKSIZE(x) if (x.size()) +// CHECK-FIXES: #define CHECKSIZE(x) if (x.size()) + +template void f() { + std::vector v; + if (v.size()) + ; + // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: The 'empty' method should be used + // CHECK-FIXES: {{^ }}if (!v.empty()){{$}} + // CHECK-FIXES-NEXT: ; + CHECKSIZE(v); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: The 'empty' method should be used + // CHECK-MESSAGES: CHECKSIZE(v); +} + +void g() { + f(); + f(); + f(); +}