Skip to content

Commit afe45ff

Browse files
PKEuSPKEuS
committed
Refactorized CheckClass::privateFunctions:
- Resolved todo about nested classes, fixed false negative, fixed wrong unit test - Removed slow and unnecessary Token::findmatch - Removed false positive when function implementation in friend class is not seen (danmar#4384)
1 parent 89cf24f commit afe45ff

File tree

2 files changed

+31
-47
lines changed

2 files changed

+31
-47
lines changed

‎lib/checkclass.cpp‎

Lines changed: 19 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -718,15 +718,24 @@ void CheckClass::suggestInitializationList(const Token* tok, const std::string&
718718
static bool checkFunctionUsage(const std::string& name, const Scope* scope)
719719
{
720720
if (!scope)
721-
return true; // Assume its used, if scope is not seen
721+
return true; // Assume it is used, if scope is not seen
722722

723723
for (std::list<Function>::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) {
724724
if (func->functionScope) {
725-
for (const Token *ftok = func->functionScope->classStart; ftok != func->functionScope->classEnd; ftok = ftok->next()) {
726-
if (ftok->str() == name && ftok->next()->str() == "(") // Function called. TODO: Handle overloads
725+
for (const Token *ftok = func->functionScope->classDef->linkAt(1); ftok != func->functionScope->classEnd; ftok = ftok->next()) {
726+
if (ftok->str() == name) // Function used. TODO: Handle overloads
727727
return true;
728728
}
729-
}
729+
} else if ((func->type != Function::eCopyConstructor &&
730+
func->type != Function::eOperatorEqual) ||
731+
func->access != Private) // Assume it is used, if a function implementation isn't seen, but empty private copy constructors and assignment operators are OK
732+
return true;
733+
}
734+
735+
for (std::list<Scope*>::const_iterator i = scope->nestedList.cbegin(); i != scope->nestedList.end(); ++i) {
736+
if ((*i)->isClassOrStruct())
737+
if (checkFunctionUsage(name, *i)) // Check nested classes, which can access private functions of their base
738+
return true;
730739
}
731740

732741
return false; // Unused in this scope
@@ -745,31 +754,11 @@ void CheckClass::privateFunctions()
745754
if (Token::findsimplematch(scope->classStart, "; __property ;", scope->classEnd))
746755
continue;
747756

748-
// check that the whole class implementation is seen
749-
bool whole = true;
750-
for (std::list<Function>::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) {
751-
if (!func->hasBody) {
752-
// empty private copy constructors and assignment operators are OK
753-
if ((func->type == Function::eCopyConstructor ||
754-
func->type == Function::eOperatorEqual) &&
755-
func->access == Private)
756-
continue;
757-
758-
whole = false;
759-
break;
760-
}
761-
}
762-
if (!whole)
763-
continue;
764-
765757
std::list<const Function*> FuncList;
766-
/** @todo embedded class have access to private functions */
767-
if (!scope->getNestedNonFunctions()) {
768-
for (std::list<Function>::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) {
769-
// Get private functions..
770-
if (func->type == Function::eFunction && func->access == Private)
771-
FuncList.push_back(&*func);
772-
}
758+
for (std::list<Function>::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) {
759+
// Get private functions..
760+
if (func->type == Function::eFunction && func->access == Private)
761+
FuncList.push_back(&*func);
773762
}
774763

775764
// Bailout for overridden virtual functions of base classes
@@ -791,23 +780,8 @@ void CheckClass::privateFunctions()
791780
for (std::list<Scope::FriendInfo>::const_iterator it = scope->friendList.begin(); !used && it != scope->friendList.end(); ++it)
792781
used = checkFunctionUsage(funcName, it->scope);
793782

794-
if (!used) {
795-
// Final check; check if the function pointer is used somewhere..
796-
const std::string _pattern("return|throw|(|)|,|= &|" + funcName);
797-
798-
// or if the function address is used somewhere...
799-
// eg. sigc::mem_fun(this, &className::classFunction)
800-
const std::string _pattern2("& " + scope->className + " :: " + funcName);
801-
const std::string methodAsArgument("(|, " + scope->className + " :: " + funcName + " ,|)");
802-
const std::string methodAssigned("%var% = &| " + scope->className + " :: " + funcName);
803-
804-
if (!Token::findmatch(_tokenizer->tokens(), _pattern.c_str()) &&
805-
!Token::findmatch(_tokenizer->tokens(), _pattern2.c_str()) &&
806-
!Token::findmatch(_tokenizer->tokens(), methodAsArgument.c_str()) &&
807-
!Token::findmatch(_tokenizer->tokens(), methodAssigned.c_str())) {
808-
unusedPrivateFunctionError(FuncList.front()->tokenDef, scope->className, funcName);
809-
}
810-
}
783+
if (!used)
784+
unusedPrivateFunctionError(FuncList.front()->tokenDef, scope->className, funcName);
811785

812786
FuncList.pop_front();
813787
}

‎test/testunusedprivfunc.cpp‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -339,11 +339,11 @@ class TestUnusedPrivateFunction : public TestFixture {
339339
" {\n"
340340
" private:\n"
341341
" };\n"
342-
"\n"
342+
"private:\n"
343343
" static void f()\n"
344344
" { }\n"
345345
"};\n");
346-
ASSERT_EQUALS("", errout.str());
346+
ASSERT_EQUALS("[test.cpp:10]: (style) Unused private function: 'A::f'\n", errout.str());
347347

348348
check("class A\n"
349349
"{\n"
@@ -452,6 +452,16 @@ class TestUnusedPrivateFunction : public TestFixture {
452452
"};");
453453
ASSERT_EQUALS("", errout.str());
454454

455+
check("struct Bar {\n"
456+
" void g() { f(); }\n" // Friend class seen, but f not seen
457+
"};\n"
458+
"class Foo {\n"
459+
"private:\n"
460+
" friend Bar;\n"
461+
" void f();\n"
462+
"};");
463+
ASSERT_EQUALS("", errout.str());
464+
455465
check("struct Bar {\n"
456466
" void g() { f(); }\n" // Friend class seen, but f() used in it
457467
"};\n"

0 commit comments

Comments
 (0)