diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index a7dd4a170..6c9203809 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -1158,11 +1158,11 @@ void CheckClass::thisSubtractionError(const Token *tok) void CheckClass::checkConst() { - // this is an inconclusive check - see #3048 for instance + // This is an inconclusive check. False positives: #3252, #3322, #3360. if (!_settings->inconclusive) return; - if (!_settings->isEnabled("style") || _settings->ifcfg) + if (!_settings->isEnabled("style")) return; // Don't check C# and JAVA classes @@ -1221,13 +1221,11 @@ void CheckClass::checkConst() } } - if (allupper && previous->str().size() > 2) + if (allupper && s.size() > 2) continue; } } - const Token *paramEnd = func->arg->link(); - // check if base class function is virtual if (!scope->derivedFrom.empty()) { if (isVirtualFunc(&(*scope), func->tokenDef)) @@ -1235,7 +1233,7 @@ void CheckClass::checkConst() } // if nothing non-const was found. write error.. - if (checkConstFunc(&(*scope), paramEnd)) { + if (checkConstFunc(&(*scope), &*func)) { std::string classname = scope->className; const Scope *nest = scope->nestedIn; while (nest && nest->type != Scope::eGlobal) { @@ -1366,46 +1364,31 @@ bool CheckClass::isConstMemberFunc(const Scope *scope, const Token *tok) return false; } -bool CheckClass::checkConstFunc(const Scope *scope, const Token *tok) +bool CheckClass::checkConstFunc(const Scope *scope, const Function *func) { // if the function doesn't have any assignment nor function call, // it can be a const function.. - unsigned int indentlevel = 0; - bool isconst = true; - for (const Token *tok1 = tok; tok1; tok1 = tok1->next()) { - if (tok1->str() == "{") - ++indentlevel; - else if (tok1->str() == "}") { - if (indentlevel <= 1) - break; - --indentlevel; - } - + for (const Token *tok1 = func->start; tok1 && tok1 != func->start->link(); tok1 = tok1->next()) { // assignment.. = += |= .. - else if (tok1->isAssignmentOp()) { + if (tok1->isAssignmentOp()) { if (tok1->next()->str() == "this") { - isconst = false; - break; + return(false); } else if (isMemberVar(scope, tok1->previous())) { - isconst = false; - break; + return(false); } } // streaming: << else if (tok1->str() == "<<" && isMemberVar(scope, tok1->previous())) { - isconst = false; - break; + return(false); } else if (Token::simpleMatch(tok1->previous(), ") <<") && isMemberVar(scope, tok1->tokAt(-2))) { - isconst = false; - break; + return(false); } // streaming: >> else if (tok1->str() == ">>" && isMemberVar(scope, tok1->next())) { - isconst = false; - break; + return(false); } // increment/decrement (member variable?).. @@ -1414,24 +1397,21 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Token *tok) if (Token::Match(tok1->previous(), "%var%") && tok1->previous()->str() != "return") { if (isMemberVar(scope, tok1->previous())) { - isconst = false; - break; + return(false); } } // var[...]++ and var[...]-- else if (tok1->previous()->str() == "]") { if (isMemberVar(scope, tok1->previous()->link()->previous())) { - isconst = false; - break; + return(false); } } // ++var and --var else if (Token::Match(tok1->next(), "%var%")) { if (isMemberVar(scope, tok1->next())) { - isconst = false; - break; + return(false); } } } @@ -1442,37 +1422,49 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Token *tok) if (var && (var->typeStartToken()->str() == "map" || Token::simpleMatch(var->typeStartToken(), "std :: map"))) { - isconst = false; - break; + return(false); } } // function call.. - else if (Token::Match(tok1, "%var% (") && - !(Token::Match(tok1, "return|c_str|if|string|switch|while|catch|for") || tok1->isStandardType())) { + else if (Token::Match(tok1, "%var% (") && !tok1->isStandardType() && + !Token::Match(tok1, "return|c_str|if|string|switch|while|catch|for")) { if (!isConstMemberFunc(scope, tok1)) { - isconst = false; - break; + return(false); + } + // Member variable given as parameter + for (const Token* tok2 = tok1->tokAt(2); tok2 && tok2 != tok1->next()->link(); tok2 = tok2->next()) { + if (tok2->str() == "(") + tok2 = tok2->link(); + else if (tok2->isName() && isMemberVar(scope, tok2)) + return(false); // TODO: Only bailout if function takes argument as non-const reference } } else if (Token::Match(tok1, "%var% < %any% > (")) { - isconst = false; - break; - } else if (Token::Match(tok1, "%var% . size|empty ( )") && tok1->varId()) { + return(false); + } else if (Token::Match(tok1, "%var% . size|empty|cend|crend|cbegin|crbegin|max_size|length|count|capacity|rfind|get_allocator|copy ( )") && tok1->varId()) { // assume all std::*::size() and std::*::empty() are const const Variable *var = symbolDatabase->getVariableFromVarId(tok1->varId()); if (var && Token::simpleMatch(var->typeStartToken(), "std ::")) tok1 = tok1->tokAt(4); + else + return(false); } // delete.. else if (tok1->str() == "delete") { - isconst = false; - break; + const Token* end = Token::findsimplematch(tok1->next(), ";")->previous(); + while (end->str() == ")") + end = end->previous(); + if (end->str() == "this") + return(false); + if (end->isName() && isMemberVar(scope, end)) + return(false); + } } - return isconst; + return(true); } // check if this function is defined virtual in the base classes diff --git a/lib/checkclass.h b/lib/checkclass.h index 40c538698..8b8b107ef 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -165,7 +165,7 @@ private: // checkConst helper functions bool isMemberVar(const Scope *scope, const Token *tok); bool isConstMemberFunc(const Scope *scope, const Token *tok); - bool checkConstFunc(const Scope *scope, const Token *tok); + bool checkConstFunc(const Scope *scope, const Function *func); /** @brief check if this function is virtual in the base classes */ bool isVirtualFunc(const Scope *scope, const Token *functionToken) const; diff --git a/test/testclass.cpp b/test/testclass.cpp index 48c18c6c2..47d0bb078 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -184,14 +184,15 @@ private: TEST_CASE(const49); // ticket #2795 TEST_CASE(const50); // ticket #2943 TEST_CASE(const51); // ticket #3040 - TEST_CASE(const52); // ticket #3049 - TEST_CASE(const53); // ticket #3052 - TEST_CASE(const54); - TEST_CASE(const55); // ticket #3149 + TEST_CASE(const52); // ticket #3048 + TEST_CASE(const53); // ticket #3049 + TEST_CASE(const54); // ticket #3052 + TEST_CASE(const55); + TEST_CASE(const56); // ticket #3149 TEST_CASE(assigningPointerToPointerIsNotAConstOperation); TEST_CASE(assigningArrayElementIsNotAConstOperation); TEST_CASE(constoperator1); // operator< can often be const - TEST_CASE(constoperator2); // operator<< + TEST_CASE(constoperator2); // operator<< TEST_CASE(constoperator3); TEST_CASE(constoperator4); TEST_CASE(constincdec); // increment/decrement => non-const @@ -3553,7 +3554,7 @@ private: "[test.cpp:3]: (warning) Suspicious pointer subtraction\n", errout.str()); } - void checkConst(const char code[], const Settings *s = 0) { + void checkConst(const char code[], const Settings *s = 0, bool inconclusive = true) { // Clear the error log errout.str(""); @@ -3563,7 +3564,7 @@ private: settings = *s; else settings.addEnabled("style"); - settings.inconclusive = true; + settings.inconclusive = inconclusive; // Tokenize.. Tokenizer tokenizer(&settings, this); @@ -5609,7 +5610,17 @@ private: ASSERT_EQUALS("", errout.str()); } - void const52() { // ticket 3049 + void const52() { // ticket 3048 + checkConst("class foo {\n" + " void DoSomething(int &a) const { a = 1; }\n" + " void DoSomethingElse() { DoSomething(bar); }\n" + "private:\n" + " int bar;\n" + "};"); + ASSERT_EQUALS("", errout.str()); + } + + void const53() { // ticket 3049 checkConst("class A {\n" " public:\n" " A() : foo(false) {};\n" @@ -5625,7 +5636,7 @@ private: ASSERT_EQUALS("", errout.str()); } - void const53() { // ticket 3052 + void const54() { // ticket 3052 checkConst("class Example {\n" " public:\n" " void Clear(void) { Example tmp; (*this) = tmp; }\n" @@ -5633,7 +5644,7 @@ private: ASSERT_EQUALS("", errout.str()); } - void const54() { + void const55() { checkConst("class MyObject {\n" " int tmp;\n" " MyObject() : tmp(0) {}\n" @@ -5643,7 +5654,7 @@ private: ASSERT_EQUALS("", errout.str()); } - void const55() { // ticket #3149 + void const56() { // ticket #3149 checkConst("class MyObject {\n" "public:\n" " void foo(int x) {\n" @@ -6350,8 +6361,11 @@ private: ASSERT_EQUALS("[test.cpp:2]: (style) Technically the member function 'foo::f' can be const.\n", errout.str()); settings.ifcfg = true; - checkConst(code, &settings); + checkConst(code, &settings, false); ASSERT_EQUALS("", errout.str()); + + checkConst(code, &settings, true); + ASSERT_EQUALS("[test.cpp:2]: (style) Technically the member function 'foo::f' can be const.\n", errout.str()); } void constFriend() { // ticket #1921 diff --git a/test/testpathmatch.cpp b/test/testpathmatch.cpp index 5d49570f9..42cc18f78 100644 --- a/test/testpathmatch.cpp +++ b/test/testpathmatch.cpp @@ -44,6 +44,10 @@ private: TEST_CASE(onemasklongerpath1); TEST_CASE(onemasklongerpath2); TEST_CASE(onemasklongerpath3); + TEST_CASE(twomasklongerpath1); + TEST_CASE(twomasklongerpath2); + TEST_CASE(twomasklongerpath3); + TEST_CASE(twomasklongerpath4); TEST_CASE(filemask1); TEST_CASE(filemaskdifferentcase); TEST_CASE(filemask2);