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 FunctionSize.cpp NamespaceCommentCheck.cpp ReadabilityTidyModule.cpp Index: clang-tidy/readability/ContainerSizeEmpty.h =================================================================== --- clang-tidy/readability/ContainerSizeEmpty.h +++ clang-tidy/readability/ContainerSizeEmpty.h @@ -0,0 +1,39 @@ +//===--- 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 size is a +/// constant-time function, and it is generally more efficient and also shows +/// clearer intent to use empty. Furthermore some containers may implement the +/// empty method but not implement the size method. Using 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,174 @@ +//===--- 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 { +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; +}(); + +bool isContainer(llvm::StringRef className) { + 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 wrong_use = 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"))), wrong_use).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. + const auto UO = Result.Nodes.getNodeAs("NegOnSize"); + if (UO) + Hint = + FixItHint::CreateReplacement(UO->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 "FunctionSize.h" #include "RedundantSmartptrGet.h" @@ -23,6 +24,8 @@ void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck( "readability-braces-around-statements"); + CheckFactories.registerCheck( + "readability-container-size-empty"); CheckFactories.registerCheck( "readability-function-size"); 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,107 @@ +// 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-1]]:5: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty] + // CHECK-MESSAGES: if(vect.size() == 0); + // CHECK-FIXES: vect.empty() + if(vect.size() != 0); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used + // CHECK-MESSAGES: if(vect.size() != 0); + // CHECK-FIXES: !vect.empty() + if(0 == vect.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: The 'empty' method should be used + // CHECK-MESSAGES: if(0 == vect.size()); + // CHECK-FIXES: vect.empty() + if(0 != vect.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: The 'empty' method should be used + // CHECK-MESSAGES: if(0 != vect.size()); + // CHECK-FIXES: !vect.empty() + if(vect.size() > 0); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used + // CHECK-MESSAGES: if(vect.size() > 0); + // CHECK-FIXES: !vect.empty() + if(0 < vect.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: The 'empty' method should be used + // CHECK-MESSAGES: if(0 < vect.size()); + // CHECK-FIXES: !vect.empty() + if(vect.size() < 1); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used + // CHECK-MESSAGES: if(vect.size() < 1); + // CHECK-FIXES: vect.empty() + if(1 > vect.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: The 'empty' method should be used + // CHECK-MESSAGES: if(1 > vect.size()); + // CHECK-FIXES: vect.empty() + if(vect.size() >= 1); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used + // CHECK-MESSAGES: if(vect.size() >= 1); + // CHECK-FIXES: !vect.empty() + if(1 <= vect.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: The 'empty' method should be used + // CHECK-MESSAGES: if(1 <= vect.size()); + // CHECK-FIXES: !vect.empty() + if(!vect.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: The 'empty' method should be used + // CHECK-MESSAGES: if(!vect.size()); + // CHECK-FIXES: vect.empty() + if(vect.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used + // CHECK-MESSAGES: if(vect.size()); + // CHECK-FIXES: !vect.empty() + + if(vect.empty()); + + const std::vector vect2; + if(vect2.size() != 0); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used + // CHECK-MESSAGES: if(vect2.size() != 0) + // CHECK-FIXES: !vect2.empty() + + std::vector *vect3 = new std::vector(); + if(vect3->size() == 0); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used + // CHECK-MESSAGES: if(vect3->size() == 0) + // CHECK-FIXES: vect3->empty() + + delete vect3; + + const std::vector &vect4 = vect2; + if(vect4.size() == 0); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used + // CHECK-MESSAGES: if(vect4.size() == 0) + // CHECK-FIXES: vect4.empty() +} + +#define CHECKSIZE(x) if (x.size()) + +template +void f() { + std::vector v; + if (v.size()) {} + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: The 'empty' method should be used + // CHECK-MESSAGES: if (v.size()) {} + // CHECK-FIXES: !v.empty() + CHECKSIZE(v); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: The 'empty' method should be used + // CHECK-MESSAGES: CHECKSIZE(v); +} + +void g() { + f(); + f(); + f(); +} +