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,38 @@ +//===--- 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 that whether a size method call can be replaced by empty +/// +/// The emptiness of a container should be checked using the empty method +/// instead of the 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,160 @@ +//===--- 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 +#include + +#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 std::set containerNames = { + "std::vector", "std::list", "std::array", "std::deque", "std::forward_list", + "std::set", "std::map", "std::multiset", "std::multimap", + "std::unordered_set", "std::unordered_map", "std::unordered_multiset", + "std::unordered_multimap", "std::stack", "std::queue", + "std::priority_queue"}; + +bool isContainer(const std::string &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 CXXMemberCallExpr *MC = + Result.Nodes.getNodeAs("SizeCallExpr"); + const BinaryOperator *BOP = + Result.Nodes.getNodeAs("SizeBinaryOp"); + const Expr *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 (BOP) { // Determine the correct transformation + bool Negation = false; + const bool ContIsLHS = !llvm::isa(BOP->getLHS()); + const auto OpCode = BOP->getOpcode(); + uint64_t Value = 0; + if (ContIsLHS) { + if (IntegerLiteral *LIT = llvm::dyn_cast(BOP->getRHS())) + Value = LIT->getValue().getLimitedValue(); + else + return; + } else { + Value = llvm::dyn_cast(BOP->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 && ContIsLHS) || + (OpCode == BinaryOperatorKind::BO_LE && Value == 0 && !ContIsLHS)) + return; + + if (OpCode == BinaryOperatorKind::BO_NE && Value == 0) + Negation = true; + if ((OpCode == BinaryOperatorKind::BO_GT || + OpCode == BinaryOperatorKind::BO_GE) && + ContIsLHS) + Negation = true; + if ((OpCode == BinaryOperatorKind::BO_LT || + OpCode == BinaryOperatorKind::BO_LE) && + !ContIsLHS) + Negation = true; + + if (Negation) + ReplacementText = "!" + ReplacementText; + hint = FixItHint::CreateReplacement(BOP->getSourceRange(), ReplacementText); + + } else { // If there is a conversion above the size call to bool, it is safe + // to just replace size with empty + const UnaryOperator *UO = + Result.Nodes.getNodeAs("NegOnSize"); + if (UO) + hint = + FixItHint::CreateReplacement(UO->getSourceRange(), ReplacementText); + else + hint = FixItHint::CreateReplacement(MC->getSourceRange(), + "!" + ReplacementText); + } + diag(MC->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: modularize/ModuleAssistant.cpp =================================================================== --- modularize/ModuleAssistant.cpp +++ modularize/ModuleAssistant.cpp @@ -68,7 +68,7 @@ // Destructor. Module::~Module() { // Free submodules. - while (SubModules.size()) { + while (!SubModules.empty()) { Module *last = SubModules.back(); SubModules.pop_back(); delete last; Index: module-map-checker/ModuleMapChecker.cpp =================================================================== --- module-map-checker/ModuleMapChecker.cpp +++ module-map-checker/ModuleMapChecker.cpp @@ -261,7 +261,7 @@ findUnaccountedForHeaders(); // Check for warnings. - if (UnaccountedForHeaders.size()) + if (!UnaccountedForHeaders.empty()) returnValue = std::error_code(1, std::generic_category()); // Dump module map if requested. 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,71 @@ +// 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-FIXES: vect.empty() + 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-FIXES: !vect.empty() + if(0 == vect.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty] + // CHECK-FIXES: vect.empty() + if(0 != vect.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty] + // CHECK-FIXES: !vect.empty() + 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-FIXES: !vect.empty() + if(0 < vect.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty] + // CHECK-FIXES: !vect.empty() + if(vect.size() < 1); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty] + // CHECK-FIXES: vect.empty() + if(1 > vect.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty] + // CHECK-FIXES: vect.empty() + if(vect.size() >= 1); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty] + // CHECK-FIXES: !vect.empty() + if(1 <= vect.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty] + // CHECK-FIXES: !vect.empty() + if(!vect.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty] + // CHECK-FIXES: vect.empty() + if(vect.size()); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: The 'empty' method should be used to check for emptiness instead of 'size'. [readability-container-size-empty] + // 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 to check for emptiness instead of 'size'. [readability-container-size-empty] + // 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 to check for emptiness instead of 'size'. [readability-container-size-empty] + // 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 to check for emptiness instead of 'size'. [readability-container-size-empty] + // CHECK-FIXES: vect4.empty() +}