From 8b29139f15333432fde34dd045cd7af0f39f0b46 Mon Sep 17 00:00:00 2001 From: Anton Lindqvist Date: Fri, 10 Jan 2025 19:29:07 +0100 Subject: [PATCH] Detect C functions that can be declared with static linkage (#7127) Currently limited to C as I assume the anonymous namespace should be favored for C++. --- lib/checkunusedfunctions.cpp | 39 ++++++++++++++++++----- lib/checkunusedfunctions.h | 2 ++ samples/AssignmentAddressToInteger/bad.c | 2 +- samples/AssignmentAddressToInteger/good.c | 2 +- samples/autoVariables/bad.c | 2 +- samples/autoVariables/good.c | 2 +- samples/incorrectLogicOperator/bad.c | 2 +- samples/incorrectLogicOperator/good.c | 2 +- test/testunusedfunctions.cpp | 16 ++++++++++ 9 files changed, 55 insertions(+), 14 deletions(-) diff --git a/lib/checkunusedfunctions.cpp b/lib/checkunusedfunctions.cpp index bf5b00d0801..9d128055a35 100644 --- a/lib/checkunusedfunctions.cpp +++ b/lib/checkunusedfunctions.cpp @@ -107,6 +107,8 @@ void CheckUnusedFunctions::parseTokens(const Tokenizer &tokenizer, const Setting if (!usage.lineNumber) usage.lineNumber = func->token->linenr(); + usage.isC = func->token->isC(); + usage.isStatic = func->isStatic(); // TODO: why always overwrite this but not the filename and line? usage.fileIndex = func->token->fileIndex(); @@ -337,6 +339,23 @@ static bool isOperatorFunction(const std::string & funcName) return std::find(additionalOperators.cbegin(), additionalOperators.cend(), funcName.substr(operatorPrefix.length())) != additionalOperators.cend(); } +static void staticFunctionError(ErrorLogger& errorLogger, + const std::string &filename, + unsigned int fileIndex, + unsigned int lineNumber, + const std::string &funcname) +{ + std::list locationList; + if (!filename.empty()) { + locationList.emplace_back(filename, lineNumber, 0); + locationList.back().fileIndex = fileIndex; + } + + const ErrorMessage errmsg(std::move(locationList), emptyString, Severity::style, "$symbol:" + funcname + "\nThe function '$symbol' should have static linkage since it is not used outside of its translation unit.", "staticFunction", Certainty::normal); + errorLogger.reportErr(errmsg); +} + + #define logChecker(id) \ do { \ const ErrorMessage errmsg({}, nullptr, Severity::internal, "logChecker", (id), CWE(0U), Certainty::normal); \ @@ -349,6 +368,7 @@ bool CheckUnusedFunctions::check(const Settings& settings, ErrorLogger& errorLog using ErrorParams = std::tuple; std::vector errors; // ensure well-defined order + std::vector staticFunctionErrors; for (auto it = mFunctions.cbegin(); it != mFunctions.cend(); ++it) { const FunctionUsage &func = it->second; @@ -363,19 +383,22 @@ bool CheckUnusedFunctions::check(const Settings& settings, ErrorLogger& errorLog if (func.filename != "+") filename = func.filename; errors.emplace_back(filename, func.fileIndex, func.lineNumber, it->first); - } else if (!func.usedOtherFile) { - /** @todo add error message "function is only used in it can be static" */ - /* - std::ostringstream errmsg; - errmsg << "The function '" << it->first << "' is only used in the file it was declared in so it should have local linkage."; - mErrorLogger->reportErr( errmsg.str() ); - errors = true; - */ + } else if (func.isC && !func.isStatic && !func.usedOtherFile) { + std::string filename; + if (func.filename != "+") + filename = func.filename; + staticFunctionErrors.emplace_back(filename, func.fileIndex, func.lineNumber, it->first); } } + std::sort(errors.begin(), errors.end()); for (const auto& e : errors) unusedFunctionError(errorLogger, std::get<0>(e), std::get<1>(e), std::get<2>(e), std::get<3>(e)); + + std::sort(staticFunctionErrors.begin(), staticFunctionErrors.end()); + for (const auto& e : staticFunctionErrors) + staticFunctionError(errorLogger, std::get<0>(e), std::get<1>(e), std::get<2>(e), std::get<3>(e)); + return !errors.empty(); } diff --git a/lib/checkunusedfunctions.h b/lib/checkunusedfunctions.h index 99d93d38dcb..6e8e1f71da8 100644 --- a/lib/checkunusedfunctions.h +++ b/lib/checkunusedfunctions.h @@ -76,6 +76,8 @@ class CPPCHECKLIB CheckUnusedFunctions { unsigned int fileIndex{}; bool usedSameFile{}; bool usedOtherFile{}; + bool isC{}; + bool isStatic{}; }; std::unordered_map mFunctions; diff --git a/samples/AssignmentAddressToInteger/bad.c b/samples/AssignmentAddressToInteger/bad.c index dcef286d6c2..0910cf0c902 100644 --- a/samples/AssignmentAddressToInteger/bad.c +++ b/samples/AssignmentAddressToInteger/bad.c @@ -1,4 +1,4 @@ -int foo(int *p) +static int foo(int *p) { int a = p; return a + 4; diff --git a/samples/AssignmentAddressToInteger/good.c b/samples/AssignmentAddressToInteger/good.c index e52e11c16b5..ca2bdd961ff 100644 --- a/samples/AssignmentAddressToInteger/good.c +++ b/samples/AssignmentAddressToInteger/good.c @@ -1,4 +1,4 @@ -int* foo(int *p) +static int* foo(int *p) { return p + 4; } diff --git a/samples/autoVariables/bad.c b/samples/autoVariables/bad.c index 0b4287d288e..9f4d0641049 100644 --- a/samples/autoVariables/bad.c +++ b/samples/autoVariables/bad.c @@ -1,4 +1,4 @@ -void foo(int **a) +static void foo(int **a) { int b = 1; *a = &b; diff --git a/samples/autoVariables/good.c b/samples/autoVariables/good.c index 6e74a8e6d56..d94869262f7 100644 --- a/samples/autoVariables/good.c +++ b/samples/autoVariables/good.c @@ -1,4 +1,4 @@ -void foo(int **a) +static void foo(int **a) { int b = 1; **a = b; diff --git a/samples/incorrectLogicOperator/bad.c b/samples/incorrectLogicOperator/bad.c index 11daaf4e4ce..c3f80d8a71f 100644 --- a/samples/incorrectLogicOperator/bad.c +++ b/samples/incorrectLogicOperator/bad.c @@ -1,5 +1,5 @@ -void foo(int x) { +static void foo(int x) { if (x >= 0 || x <= 10) {} } diff --git a/samples/incorrectLogicOperator/good.c b/samples/incorrectLogicOperator/good.c index a2d122fb128..0dc96bd8a28 100644 --- a/samples/incorrectLogicOperator/good.c +++ b/samples/incorrectLogicOperator/good.c @@ -1,5 +1,5 @@ -void foo(int x) { +static void foo(int x) { if (x >= 0 && x <= 10) {} } diff --git a/test/testunusedfunctions.cpp b/test/testunusedfunctions.cpp index 9fc1ea9140d..b0b6a631bb3 100644 --- a/test/testunusedfunctions.cpp +++ b/test/testunusedfunctions.cpp @@ -88,6 +88,7 @@ class TestUnusedFunctions : public TestFixture { TEST_CASE(attributeCleanup); TEST_CASE(attributeUnused); TEST_CASE(attributeMaybeUnused); + TEST_CASE(staticFunction); } #define check(...) check_(__FILE__, __LINE__, __VA_ARGS__) @@ -820,6 +821,21 @@ class TestUnusedFunctions : public TestFixture { check("[[maybe_unused]] void f() {}\n"); ASSERT_EQUALS("", errout_str()); } + + void staticFunction() + { + check("void f(void) {}\n" + "int main() {\n" + " f();\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + check("void f(void) {}\n" + "int main() {\n" + " f();\n" + "}\n", Platform::Type::Native, nullptr, false); + ASSERT_EQUALS("[test.c:1]: (style) The function 'f' should have static linkage since it is not used outside of its translation unit.\n", errout_str()); + } }; REGISTER_TEST(TestUnusedFunctions)