From 7d5594ef08eadb172b76f34caa6a6dc3f510c39f Mon Sep 17 00:00:00 2001 From: Sergio Martins Date: Sun, 4 Mar 2018 18:09:29 +0000 Subject: Move reserve-candiates to level3 As it has quite a few false-positives --- src/Checks.h | 4 +- src/checks/level2/README-reserve-candidates.md | 33 --- src/checks/level2/reserve-candidates.cpp | 326 ------------------------- src/checks/level2/reserve-candidates.h | 64 ----- src/checks/level3/README-reserve-candidates.md | 33 +++ src/checks/level3/reserve-candidates.cpp | 326 +++++++++++++++++++++++++ src/checks/level3/reserve-candidates.h | 64 +++++ 7 files changed, 425 insertions(+), 425 deletions(-) delete mode 100644 src/checks/level2/README-reserve-candidates.md delete mode 100644 src/checks/level2/reserve-candidates.cpp delete mode 100644 src/checks/level2/reserve-candidates.h create mode 100644 src/checks/level3/README-reserve-candidates.md create mode 100644 src/checks/level3/reserve-candidates.cpp create mode 100644 src/checks/level3/reserve-candidates.h (limited to 'src') diff --git a/src/Checks.h b/src/Checks.h index b653bf1a..b819036c 100644 --- a/src/Checks.h +++ b/src/Checks.h @@ -93,12 +93,12 @@ #include "checks/level2/missing-typeinfo.h" #include "checks/level2/old-style-connect.h" #include "checks/level2/qstring-allocations.h" -#include "checks/level2/reserve-candidates.h" #include "checks/level2/returning-void-expression.h" #include "checks/level2/rule-of-three.h" #include "checks/level2/virtual-call-ctor.h" #include "checks/level3/assert-with-side-effects.h" #include "checks/level3/detaching-member.h" +#include "checks/level3/reserve-candidates.h" #include "checks/level3/thread-with-slots.h" template @@ -188,11 +188,11 @@ void CheckManager::registerChecks() registerFixIt(1, "fix-qlatin1string-allocations", "qstring-allocations"); registerFixIt(2, "fix-fromLatin1_fromUtf8-allocations", "qstring-allocations"); registerFixIt(4, "fix-fromCharPtrAllocations", "qstring-allocations"); - registerCheck(check("reserve-candidates", CheckLevel2, RegisteredCheck::Option_VisitsStmts)); registerCheck(check("returning-void-expression", CheckLevel2, RegisteredCheck::Option_VisitsStmts)); registerCheck(check("rule-of-three", CheckLevel2, RegisteredCheck::Option_VisitsDecls)); registerCheck(check("virtual-call-ctor", CheckLevel2, RegisteredCheck::Option_VisitsDecls)); registerCheck(check("assert-with-side-effects", CheckLevel3, RegisteredCheck::Option_VisitsStmts)); registerCheck(check("detaching-member", CheckLevel3, RegisteredCheck::Option_VisitsStmts)); + registerCheck(check("reserve-candidates", CheckLevel3, RegisteredCheck::Option_VisitsStmts)); registerCheck(check("thread-with-slots", CheckLevel3, RegisteredCheck::Option_VisitsStmts | RegisteredCheck::Option_VisitsDecls)); } diff --git a/src/checks/level2/README-reserve-candidates.md b/src/checks/level2/README-reserve-candidates.md deleted file mode 100644 index 6a2c6876..00000000 --- a/src/checks/level2/README-reserve-candidates.md +++ /dev/null @@ -1,33 +0,0 @@ -# reserve-candidates - - -Finds places that could use a `reserve()` call. -Whenever you know how many elements a container will hold you should reserve -space in order to avoid repeated memory allocations. - -#### Trivial example missing reserve() - - QList ages; - // list.reserve(people.size()); - for (auto person : people) - list << person.age(); - -Example where reserve shouldn't be used: - - QLost list; - for (int i = 0; i < 1000; ++i) { - // reserve() will be called 1000 times, meaning 1000 allocations - // whilst without a reserve the internal exponential growth algorithm would do a better job - list.reserve(list.size() + 2); - for (int j = 0; j < 2; ++j) { - list << m; - } - } - -#### Supported containers -`QVector`, `std::vector`, `QList`, `QSet` and `QVarLengthArray` - -#### Pitfalls -Rate of false-positives is around 15%. Don't go blindly calling `reserve()` without proper analysis. -In doubt don't use it, all containers have a growth curve and usually only do log(N) allocations -when you append N items. diff --git a/src/checks/level2/reserve-candidates.cpp b/src/checks/level2/reserve-candidates.cpp deleted file mode 100644 index 5e20c705..00000000 --- a/src/checks/level2/reserve-candidates.cpp +++ /dev/null @@ -1,326 +0,0 @@ -/* - This file is part of the clazy static checker. - - Copyright (C) 2015 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com - Author: Sérgio Martins - - Copyright (C) 2015-2016 Sergio Martins - - This library is free software; you can redistribute it and/or - modify it under the terms of the GNU Library General Public - License as published by the Free Software Foundation; either - version 2 of the License, or (at your option) any later version. - - This library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Library General Public License for more details. - - You should have received a copy of the GNU Library General Public License - along with this library; see the file COPYING.LIB. If not, write to - the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, - Boston, MA 02110-1301, USA. -*/ - -#include "reserve-candidates.h" -#include "ClazyContext.h" -#include "Utils.h" -#include "clazy_stl.h" -#include "MacroUtils.h" -#include "StringUtils.h" -#include "QtUtils.h" -#include "ContextUtils.h" -#include "HierarchyUtils.h" -#include "LoopUtils.h" -#include "ClazyContext.h" - -#include -#include -#include -#include -#include -#include - -#include - -using namespace clang; -using namespace std; - -ReserveCandidates::ReserveCandidates(const std::string &name, ClazyContext *context) - : CheckBase(name, context, Option_CanIgnoreIncludes) -{ -} - -static bool paramIsSameTypeAs(const Type *paramType, CXXRecordDecl *classDecl) -{ - if (!paramType || !classDecl) - return false; - - if (paramType->getAsCXXRecordDecl() == classDecl) - return true; - - const CXXRecordDecl *paramClassDecl = paramType->getPointeeCXXRecordDecl(); - return paramClassDecl && paramClassDecl == classDecl; -} - -static bool isCandidateMethod(CXXMethodDecl *methodDecl) -{ - if (!methodDecl) - return false; - - CXXRecordDecl *classDecl = methodDecl->getParent(); - if (!classDecl) - return false; - - if (!clazy::equalsAny(clazy::name(methodDecl), { "append", "push_back", "push", "operator<<", "operator+=" })) - return false; - - if (!clazy::isAReserveClass(classDecl)) - return false; - - // Catch cases like: QList::append(const QList &), which don't make sense to reserve. - // In this case, the parameter has the same type of the class - ParmVarDecl *parm = methodDecl->getParamDecl(0); - if (paramIsSameTypeAs(parm->getType().getTypePtrOrNull(), classDecl)) - return false; - - return true; -} - -static bool isCandidate(CallExpr *oper) -{ - if (!oper) - return false; - - return isCandidateMethod(dyn_cast_or_null(oper->getDirectCallee())); -} - -bool ReserveCandidates::containerWasReserved(clang::ValueDecl *valueDecl) const -{ - return valueDecl && clazy::contains(m_foundReserves, valueDecl); -} - -bool ReserveCandidates::acceptsValueDecl(ValueDecl *valueDecl) const -{ - // Rules: - // 1. The container variable must have been defined inside a function. Too many false positives otherwise. - // free to comment that out and go through the results, maybe you'll find something. - - // 2. If we found at least one reserve call, lets not warn about it. - - if (!valueDecl || isa(valueDecl) || containerWasReserved(valueDecl)) - return false; - - if (clazy::isValueDeclInFunctionContext(valueDecl)) - return true; - - // Actually, lets allow for some member variables containers if they are being used inside CTORs or DTORs - // Those functions are only called once, so it's OK. For other member functions it's dangerous and needs - // human inspection, if such member function would be called in a loop we would be constantly calling reserve - // and in that case the built-in exponential growth is better. - - if (!m_context->lastMethodDecl || !(isa(m_context->lastMethodDecl) || isa(m_context->lastMethodDecl))) - return false; - - CXXRecordDecl *record = Utils::isMemberVariable(valueDecl); - if (record && m_context->lastMethodDecl->getParent() == record) - return true; - - return false; -} - -bool ReserveCandidates::isReserveCandidate(ValueDecl *valueDecl, Stmt *loopBody, CallExpr *callExpr) const -{ - if (!acceptsValueDecl(valueDecl)) - return false; - - const bool isMemberVariable = Utils::isMemberVariable(valueDecl); - // We only want containers defined outside of the loop we're examining - if (!isMemberVariable && sm().isBeforeInSLocAddrSpace(loopBody->getLocStart(), valueDecl->getLocStart())) - return false; - - if (isInComplexLoop(callExpr, valueDecl->getLocStart(), isMemberVariable)) - return false; - - if (clazy::loopCanBeInterrupted(loopBody, m_context->sm, callExpr->getLocStart())) - return false; - - return true; -} - -void ReserveCandidates::VisitStmt(clang::Stmt *stm) -{ - if (registerReserveStatement(stm)) - return; - - auto body = clazy::bodyFromLoop(stm); - if (!body) - return; - - const bool isForeach = clazy::isInMacro(&m_astContext, stm->getLocStart(), "Q_FOREACH"); - - // If the body is another loop, we have nesting, ignore it now since the inner loops will be visited soon. - if (isa(body) || isa(body) || (!isForeach && isa(body))) - return; - - // TODO: Search in both branches of the if statement - if (isa(body)) - return; - - // Get the list of member calls and operator<< that are direct childs of the loop statements - // If it's inside an if statement we don't care. - auto callExprs = clazy::getStatements(body, nullptr, {}, /*depth=*/ 1, - /*includeParent=*/ true, - clazy::IgnoreExprWithCleanups); - - - for (CallExpr *callExpr : callExprs) { - if (!isCandidate(callExpr)) - continue; - - ValueDecl *valueDecl = Utils::valueDeclForCallExpr(callExpr); - if (isReserveCandidate(valueDecl, body, callExpr)) - emitWarning(callExpr->getLocStart(), "Reserve candidate"); - } -} - -// Catch existing reserves -bool ReserveCandidates::registerReserveStatement(Stmt *stm) -{ - auto memberCall = dyn_cast(stm); - if (!memberCall) - return false; - - CXXMethodDecl *methodDecl = memberCall->getMethodDecl(); - if (!methodDecl || clazy::name(methodDecl) != "reserve") - return false; - - CXXRecordDecl *decl = methodDecl->getParent(); - if (!clazy::isAReserveClass(decl)) - return false; - - ValueDecl *valueDecl = Utils::valueDeclForMemberCall(memberCall); - if (!valueDecl) - return false; - - if (!clazy::contains(m_foundReserves, valueDecl)) - m_foundReserves.push_back(valueDecl); - - return true; -} - -bool ReserveCandidates::expressionIsComplex(clang::Expr *expr) const -{ - if (!expr) - return false; - - vector callExprs; - clazy::getChilds(expr, callExprs); - - for (CallExpr *callExpr : callExprs) { - if (clazy::isJavaIterator(dyn_cast(callExpr))) - continue; - - QualType qt = callExpr->getType(); - const Type *t = qt.getTypePtrOrNull(); - if (t && (!t->isIntegerType() || t->isBooleanType())) - return true; - } - - vector subscriptExprs; - clazy::getChilds(expr, subscriptExprs); - if (!subscriptExprs.empty()) - return true; - - BinaryOperator* binary = dyn_cast(expr); - if (binary && binary->isAssignmentOp()) { // Filter things like for ( ...; ...; next = node->next) - - Expr *rhs = binary->getRHS(); - if (isa(rhs) || (isa(rhs) && dyn_cast_or_null(clazy::getFirstChildAtDepth(rhs, 1)))) - return true; - } - - // llvm::errs() << expr->getStmtClassName() << "\n"; - return false; -} - -bool ReserveCandidates::loopIsComplex(clang::Stmt *stm, bool &isLoop) const -{ - isLoop = false; - - if (auto forstm = dyn_cast(stm)) { - isLoop = true; - return !forstm->getCond() || !forstm->getInc() || expressionIsComplex(forstm->getCond()) || expressionIsComplex(forstm->getInc()); - } - - if (isa(stm)) { - isLoop = true; - return false; - } - - if (dyn_cast(stm) || dyn_cast(stm)) { - // Too many false-positives with while statements. Ignore it. - isLoop = true; - return true; - } - - return false; -} - -bool ReserveCandidates::isInComplexLoop(clang::Stmt *s, SourceLocation declLocation, bool isMemberVariable) const -{ - if (!s || declLocation.isInvalid()) - return false; - - int forCount = 0; - int foreachCount = 0; - - static vector nonComplexOnesCache; - static vector complexOnesCache; - auto rawLoc = s->getLocStart().getRawEncoding(); - - - // For some reason we generate two warnings on some foreaches, so cache the ones we processed - // and return true so we don't trigger a warning - if (clazy::contains(nonComplexOnesCache, rawLoc) || clazy::contains(complexOnesCache, rawLoc)) - return true; - - Stmt *parent = s; - PresumedLoc lastForeachForStm; - while ((parent = clazy::parent(m_context->parentMap, parent))) { - const SourceLocation parentStart = parent->getLocStart(); - if (!isMemberVariable && sm().isBeforeInSLocAddrSpace(parentStart, declLocation)) { - nonComplexOnesCache.push_back(rawLoc); - return false; - } - - bool isLoop = false; - if (loopIsComplex(parent, isLoop)) { - complexOnesCache.push_back(rawLoc); - return true; - } - - if (clazy::isInForeach(&m_astContext, parentStart)) { - auto ploc = sm().getPresumedLoc(parentStart); - if (Utils::presumedLocationsEqual(ploc, lastForeachForStm)) { - // Q_FOREACH comes in pairs, because each has two for statements inside, so ignore one when counting - } else { - foreachCount++; - lastForeachForStm = ploc; - } - } else { - if (isLoop) - forCount++; - } - - if (foreachCount > 1 || forCount > 1) { // two foreaches are almost always a false-positve - complexOnesCache.push_back(rawLoc); - return true; - } - - - } - - nonComplexOnesCache.push_back(rawLoc); - return false; -} diff --git a/src/checks/level2/reserve-candidates.h b/src/checks/level2/reserve-candidates.h deleted file mode 100644 index 9b9ed5eb..00000000 --- a/src/checks/level2/reserve-candidates.h +++ /dev/null @@ -1,64 +0,0 @@ -/* - This file is part of the clazy static checker. - - Copyright (C) 2015 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com - Author: Sérgio Martins - - Copyright (C) 2015 Sergio Martins - - This library is free software; you can redistribute it and/or - modify it under the terms of the GNU Library General Public - License as published by the Free Software Foundation; either - version 2 of the License, or (at your option) any later version. - - This library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Library General Public License for more details. - - You should have received a copy of the GNU Library General Public License - along with this library; see the file COPYING.LIB. If not, write to - the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, - Boston, MA 02110-1301, USA. -*/ - -#ifndef CLAZY_RESERVE_CANDIDATES -#define CLAZY_RESERVE_CANDIDATES - -#include "checkbase.h" - -#include - -namespace clang { -class ValueDecl; -class Expr; -class CallExpr; -} - -/** - * Recommends places that are missing QList::reserve() or QVector::reserve(). - * - * Only local variables are contemplated, containers that are members of a class are ignored due to - * high false-positive rate. - * - * There some chance of false-positives. - */ -class ReserveCandidates : public CheckBase -{ -public: - ReserveCandidates(const std::string &name, ClazyContext *context); - void VisitStmt(clang::Stmt *stm) override; - -private: - bool registerReserveStatement(clang::Stmt *stmt); - bool containerWasReserved(clang::ValueDecl*) const; - bool acceptsValueDecl(clang::ValueDecl *valueDecl) const; - bool expressionIsComplex(clang::Expr *) const; - bool loopIsComplex(clang::Stmt *, bool &isLoop) const; - bool isInComplexLoop(clang::Stmt *, clang::SourceLocation declLocation, bool isMemberVariable) const; - bool isReserveCandidate(clang::ValueDecl *valueDecl, clang::Stmt *loopBody, clang::CallExpr *callExpr) const; - - std::vector m_foundReserves; -}; - -#endif diff --git a/src/checks/level3/README-reserve-candidates.md b/src/checks/level3/README-reserve-candidates.md new file mode 100644 index 00000000..6a2c6876 --- /dev/null +++ b/src/checks/level3/README-reserve-candidates.md @@ -0,0 +1,33 @@ +# reserve-candidates + + +Finds places that could use a `reserve()` call. +Whenever you know how many elements a container will hold you should reserve +space in order to avoid repeated memory allocations. + +#### Trivial example missing reserve() + + QList ages; + // list.reserve(people.size()); + for (auto person : people) + list << person.age(); + +Example where reserve shouldn't be used: + + QLost list; + for (int i = 0; i < 1000; ++i) { + // reserve() will be called 1000 times, meaning 1000 allocations + // whilst without a reserve the internal exponential growth algorithm would do a better job + list.reserve(list.size() + 2); + for (int j = 0; j < 2; ++j) { + list << m; + } + } + +#### Supported containers +`QVector`, `std::vector`, `QList`, `QSet` and `QVarLengthArray` + +#### Pitfalls +Rate of false-positives is around 15%. Don't go blindly calling `reserve()` without proper analysis. +In doubt don't use it, all containers have a growth curve and usually only do log(N) allocations +when you append N items. diff --git a/src/checks/level3/reserve-candidates.cpp b/src/checks/level3/reserve-candidates.cpp new file mode 100644 index 00000000..5e20c705 --- /dev/null +++ b/src/checks/level3/reserve-candidates.cpp @@ -0,0 +1,326 @@ +/* + This file is part of the clazy static checker. + + Copyright (C) 2015 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com + Author: Sérgio Martins + + Copyright (C) 2015-2016 Sergio Martins + + This library is free software; you can redistribute it and/or + modify it under the terms of the GNU Library General Public + License as published by the Free Software Foundation; either + version 2 of the License, or (at your option) any later version. + + This library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Library General Public License for more details. + + You should have received a copy of the GNU Library General Public License + along with this library; see the file COPYING.LIB. If not, write to + the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, + Boston, MA 02110-1301, USA. +*/ + +#include "reserve-candidates.h" +#include "ClazyContext.h" +#include "Utils.h" +#include "clazy_stl.h" +#include "MacroUtils.h" +#include "StringUtils.h" +#include "QtUtils.h" +#include "ContextUtils.h" +#include "HierarchyUtils.h" +#include "LoopUtils.h" +#include "ClazyContext.h" + +#include +#include +#include +#include +#include +#include + +#include + +using namespace clang; +using namespace std; + +ReserveCandidates::ReserveCandidates(const std::string &name, ClazyContext *context) + : CheckBase(name, context, Option_CanIgnoreIncludes) +{ +} + +static bool paramIsSameTypeAs(const Type *paramType, CXXRecordDecl *classDecl) +{ + if (!paramType || !classDecl) + return false; + + if (paramType->getAsCXXRecordDecl() == classDecl) + return true; + + const CXXRecordDecl *paramClassDecl = paramType->getPointeeCXXRecordDecl(); + return paramClassDecl && paramClassDecl == classDecl; +} + +static bool isCandidateMethod(CXXMethodDecl *methodDecl) +{ + if (!methodDecl) + return false; + + CXXRecordDecl *classDecl = methodDecl->getParent(); + if (!classDecl) + return false; + + if (!clazy::equalsAny(clazy::name(methodDecl), { "append", "push_back", "push", "operator<<", "operator+=" })) + return false; + + if (!clazy::isAReserveClass(classDecl)) + return false; + + // Catch cases like: QList::append(const QList &), which don't make sense to reserve. + // In this case, the parameter has the same type of the class + ParmVarDecl *parm = methodDecl->getParamDecl(0); + if (paramIsSameTypeAs(parm->getType().getTypePtrOrNull(), classDecl)) + return false; + + return true; +} + +static bool isCandidate(CallExpr *oper) +{ + if (!oper) + return false; + + return isCandidateMethod(dyn_cast_or_null(oper->getDirectCallee())); +} + +bool ReserveCandidates::containerWasReserved(clang::ValueDecl *valueDecl) const +{ + return valueDecl && clazy::contains(m_foundReserves, valueDecl); +} + +bool ReserveCandidates::acceptsValueDecl(ValueDecl *valueDecl) const +{ + // Rules: + // 1. The container variable must have been defined inside a function. Too many false positives otherwise. + // free to comment that out and go through the results, maybe you'll find something. + + // 2. If we found at least one reserve call, lets not warn about it. + + if (!valueDecl || isa(valueDecl) || containerWasReserved(valueDecl)) + return false; + + if (clazy::isValueDeclInFunctionContext(valueDecl)) + return true; + + // Actually, lets allow for some member variables containers if they are being used inside CTORs or DTORs + // Those functions are only called once, so it's OK. For other member functions it's dangerous and needs + // human inspection, if such member function would be called in a loop we would be constantly calling reserve + // and in that case the built-in exponential growth is better. + + if (!m_context->lastMethodDecl || !(isa(m_context->lastMethodDecl) || isa(m_context->lastMethodDecl))) + return false; + + CXXRecordDecl *record = Utils::isMemberVariable(valueDecl); + if (record && m_context->lastMethodDecl->getParent() == record) + return true; + + return false; +} + +bool ReserveCandidates::isReserveCandidate(ValueDecl *valueDecl, Stmt *loopBody, CallExpr *callExpr) const +{ + if (!acceptsValueDecl(valueDecl)) + return false; + + const bool isMemberVariable = Utils::isMemberVariable(valueDecl); + // We only want containers defined outside of the loop we're examining + if (!isMemberVariable && sm().isBeforeInSLocAddrSpace(loopBody->getLocStart(), valueDecl->getLocStart())) + return false; + + if (isInComplexLoop(callExpr, valueDecl->getLocStart(), isMemberVariable)) + return false; + + if (clazy::loopCanBeInterrupted(loopBody, m_context->sm, callExpr->getLocStart())) + return false; + + return true; +} + +void ReserveCandidates::VisitStmt(clang::Stmt *stm) +{ + if (registerReserveStatement(stm)) + return; + + auto body = clazy::bodyFromLoop(stm); + if (!body) + return; + + const bool isForeach = clazy::isInMacro(&m_astContext, stm->getLocStart(), "Q_FOREACH"); + + // If the body is another loop, we have nesting, ignore it now since the inner loops will be visited soon. + if (isa(body) || isa(body) || (!isForeach && isa(body))) + return; + + // TODO: Search in both branches of the if statement + if (isa(body)) + return; + + // Get the list of member calls and operator<< that are direct childs of the loop statements + // If it's inside an if statement we don't care. + auto callExprs = clazy::getStatements(body, nullptr, {}, /*depth=*/ 1, + /*includeParent=*/ true, + clazy::IgnoreExprWithCleanups); + + + for (CallExpr *callExpr : callExprs) { + if (!isCandidate(callExpr)) + continue; + + ValueDecl *valueDecl = Utils::valueDeclForCallExpr(callExpr); + if (isReserveCandidate(valueDecl, body, callExpr)) + emitWarning(callExpr->getLocStart(), "Reserve candidate"); + } +} + +// Catch existing reserves +bool ReserveCandidates::registerReserveStatement(Stmt *stm) +{ + auto memberCall = dyn_cast(stm); + if (!memberCall) + return false; + + CXXMethodDecl *methodDecl = memberCall->getMethodDecl(); + if (!methodDecl || clazy::name(methodDecl) != "reserve") + return false; + + CXXRecordDecl *decl = methodDecl->getParent(); + if (!clazy::isAReserveClass(decl)) + return false; + + ValueDecl *valueDecl = Utils::valueDeclForMemberCall(memberCall); + if (!valueDecl) + return false; + + if (!clazy::contains(m_foundReserves, valueDecl)) + m_foundReserves.push_back(valueDecl); + + return true; +} + +bool ReserveCandidates::expressionIsComplex(clang::Expr *expr) const +{ + if (!expr) + return false; + + vector callExprs; + clazy::getChilds(expr, callExprs); + + for (CallExpr *callExpr : callExprs) { + if (clazy::isJavaIterator(dyn_cast(callExpr))) + continue; + + QualType qt = callExpr->getType(); + const Type *t = qt.getTypePtrOrNull(); + if (t && (!t->isIntegerType() || t->isBooleanType())) + return true; + } + + vector subscriptExprs; + clazy::getChilds(expr, subscriptExprs); + if (!subscriptExprs.empty()) + return true; + + BinaryOperator* binary = dyn_cast(expr); + if (binary && binary->isAssignmentOp()) { // Filter things like for ( ...; ...; next = node->next) + + Expr *rhs = binary->getRHS(); + if (isa(rhs) || (isa(rhs) && dyn_cast_or_null(clazy::getFirstChildAtDepth(rhs, 1)))) + return true; + } + + // llvm::errs() << expr->getStmtClassName() << "\n"; + return false; +} + +bool ReserveCandidates::loopIsComplex(clang::Stmt *stm, bool &isLoop) const +{ + isLoop = false; + + if (auto forstm = dyn_cast(stm)) { + isLoop = true; + return !forstm->getCond() || !forstm->getInc() || expressionIsComplex(forstm->getCond()) || expressionIsComplex(forstm->getInc()); + } + + if (isa(stm)) { + isLoop = true; + return false; + } + + if (dyn_cast(stm) || dyn_cast(stm)) { + // Too many false-positives with while statements. Ignore it. + isLoop = true; + return true; + } + + return false; +} + +bool ReserveCandidates::isInComplexLoop(clang::Stmt *s, SourceLocation declLocation, bool isMemberVariable) const +{ + if (!s || declLocation.isInvalid()) + return false; + + int forCount = 0; + int foreachCount = 0; + + static vector nonComplexOnesCache; + static vector complexOnesCache; + auto rawLoc = s->getLocStart().getRawEncoding(); + + + // For some reason we generate two warnings on some foreaches, so cache the ones we processed + // and return true so we don't trigger a warning + if (clazy::contains(nonComplexOnesCache, rawLoc) || clazy::contains(complexOnesCache, rawLoc)) + return true; + + Stmt *parent = s; + PresumedLoc lastForeachForStm; + while ((parent = clazy::parent(m_context->parentMap, parent))) { + const SourceLocation parentStart = parent->getLocStart(); + if (!isMemberVariable && sm().isBeforeInSLocAddrSpace(parentStart, declLocation)) { + nonComplexOnesCache.push_back(rawLoc); + return false; + } + + bool isLoop = false; + if (loopIsComplex(parent, isLoop)) { + complexOnesCache.push_back(rawLoc); + return true; + } + + if (clazy::isInForeach(&m_astContext, parentStart)) { + auto ploc = sm().getPresumedLoc(parentStart); + if (Utils::presumedLocationsEqual(ploc, lastForeachForStm)) { + // Q_FOREACH comes in pairs, because each has two for statements inside, so ignore one when counting + } else { + foreachCount++; + lastForeachForStm = ploc; + } + } else { + if (isLoop) + forCount++; + } + + if (foreachCount > 1 || forCount > 1) { // two foreaches are almost always a false-positve + complexOnesCache.push_back(rawLoc); + return true; + } + + + } + + nonComplexOnesCache.push_back(rawLoc); + return false; +} diff --git a/src/checks/level3/reserve-candidates.h b/src/checks/level3/reserve-candidates.h new file mode 100644 index 00000000..9b9ed5eb --- /dev/null +++ b/src/checks/level3/reserve-candidates.h @@ -0,0 +1,64 @@ +/* + This file is part of the clazy static checker. + + Copyright (C) 2015 Klarälvdalens Datakonsult AB, a KDAB Group company, info@kdab.com + Author: Sérgio Martins + + Copyright (C) 2015 Sergio Martins + + This library is free software; you can redistribute it and/or + modify it under the terms of the GNU Library General Public + License as published by the Free Software Foundation; either + version 2 of the License, or (at your option) any later version. + + This library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Library General Public License for more details. + + You should have received a copy of the GNU Library General Public License + along with this library; see the file COPYING.LIB. If not, write to + the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, + Boston, MA 02110-1301, USA. +*/ + +#ifndef CLAZY_RESERVE_CANDIDATES +#define CLAZY_RESERVE_CANDIDATES + +#include "checkbase.h" + +#include + +namespace clang { +class ValueDecl; +class Expr; +class CallExpr; +} + +/** + * Recommends places that are missing QList::reserve() or QVector::reserve(). + * + * Only local variables are contemplated, containers that are members of a class are ignored due to + * high false-positive rate. + * + * There some chance of false-positives. + */ +class ReserveCandidates : public CheckBase +{ +public: + ReserveCandidates(const std::string &name, ClazyContext *context); + void VisitStmt(clang::Stmt *stm) override; + +private: + bool registerReserveStatement(clang::Stmt *stmt); + bool containerWasReserved(clang::ValueDecl*) const; + bool acceptsValueDecl(clang::ValueDecl *valueDecl) const; + bool expressionIsComplex(clang::Expr *) const; + bool loopIsComplex(clang::Stmt *, bool &isLoop) const; + bool isInComplexLoop(clang::Stmt *, clang::SourceLocation declLocation, bool isMemberVariable) const; + bool isReserveCandidate(clang::ValueDecl *valueDecl, clang::Stmt *loopBody, clang::CallExpr *callExpr) const; + + std::vector m_foundReserves; +}; + +#endif -- cgit v1.2.3