diff --git a/.travis.yml b/.travis.yml index 0440350e2..a4f9fe87e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -45,9 +45,11 @@ script: # fail the entire job as soon as one of the subcommands exits non-zero to save time and resources - set -e # download clang git, compile cppcheck, run cppcheck on clang code to look for crashes in cppcheck. if this is done, terminate build - - if [[ "$CHECK_CLANG" == "yes" ]] && [[ "$CC" == "clang" ]]; then wget "https://github.com/llvm-mirror/clang/archive/a1f8bd3778cc5a53236a53500c6ab184e945eefa.zip" & make -j 4 & wait; unzip a1f8bd3778cc5a53236a53500c6ab184e945eefa.zip > /dev/null; touch /tmp/clang.cppcheck; cd ./clang-a1f8bd3778cc5a53236a53500c6ab184e945eefa ; ${CPPCHECK} . --max-configs=1 --enable=all --inconclusive --exception-handling --template="{callstack} ({severity}) {message} [{id}]" -iINPUTS -itest/Driver/Inputs/gen-response.c -itest/Index/index-many-logical-ops.c -itest/Sema/many-logical-ops.c --suppressions-list=../.travis_llvmcheck_suppressions -j 2 |& grep -v ".* files checked.*done" |& tee /tmp/clang.cppcheck; cd ../ ; echo "CLANG" ; ! grep "process crashed with signal\|Internal error\. compiled" /tmp/clang.cppcheck; exit; fi +# DM: I can't make it work. Let's rely on daca@home instead. +# - if [[ "$CHECK_CLANG" == "yes" ]] && [[ "$CC" == "clang" ]]; then wget "https://github.com/llvm-mirror/clang/archive/a1f8bd3778cc5a53236a53500c6ab184e945eefa.zip" & make -j 4 & wait; unzip a1f8bd3778cc5a53236a53500c6ab184e945eefa.zip > /dev/null; touch /tmp/clang.cppcheck; cd ./clang-a1f8bd3778cc5a53236a53500c6ab184e945eefa ; ${CPPCHECK} . --max-configs=1 --enable=all --inconclusive --exception-handling --template="{callstack} ({severity}) {message} [{id}]" -q -iINPUTS -itest/Driver/Inputs/gen-response.c -itest/Index/index-many-logical-ops.c -itest/Sema/many-logical-ops.c -j >& /tmp/clang.cppcheck ; cd ../ ; echo "CLANG" ; ! grep "process crashed with signal\|Internal error\. compiled" /tmp/clang.cppcheck; exit; fi # check llvm as well - - if [[ "$CHECK_LLVM" == "yes" ]] && [[ "$CC" == "clang" ]]; then wget "https://github.com/llvm-mirror/llvm/archive/6fa6453210fa34c1c28bd73a431d04734549f0d6.zip" & make -j 4 & wait; unzip 6fa6453210fa34c1c28bd73a431d04734549f0d6.zip > /dev/null; touch /tmp/llvm.cppcheck; cd ./llvm-6fa6453210fa34c1c28bd73a431d04734549f0d6 ; ${CPPCHECK} . --max-configs=1 --enable=all --inconclusive --exception-handling --template="{callstack} ({severity}) {message} [{id}]" --suppressions-list=../.travis_llvmcheck_suppressions -j 2 |& grep -v ".* files checked.*done" |& tee /tmp/llvm.cppcheck; cd ../ ; echo "LLVM" ; ! grep "process crashed with signal\|Internal error\. compiled" /tmp/llvm.cppcheck; exit; fi +# DM: I can't make it work. Let's rely on daca@home instead. +# - if [[ "$CHECK_LLVM" == "yes" ]] && [[ "$CC" == "clang" ]]; then wget "https://github.com/llvm-mirror/llvm/archive/6fa6453210fa34c1c28bd73a431d04734549f0d6.zip" & make -j 4 & wait; unzip 6fa6453210fa34c1c28bd73a431d04734549f0d6.zip > /dev/null; touch /tmp/llvm.cppcheck; cd ./llvm-6fa6453210fa34c1c28bd73a431d04734549f0d6 ; ${CPPCHECK} . --max-configs=1 --enable=all --inconclusive --exception-handling --template="{callstack} ({severity}) {message} [{id}]" --suppressions-list=../.travis_llvmcheck_suppressions -j 2 -q >& /tmp/llvm.cppcheck; cd ../ ; echo "LLVM" ; ! grep "process crashed with signal\|Internal error\. compiled" /tmp/llvm.cppcheck; exit; fi # check if dmake needs to be rerun but if yes, don't fail the build but notify us. # to update dmake: "make dmake; ./dmake; and commit - echo "If the following command fails, run 'make dmake; make run-dmake' and commit the resulting change." diff --git a/.travis_suppressions b/.travis_suppressions index 111d1f8b8..d03b1faa2 100644 --- a/.travis_suppressions +++ b/.travis_suppressions @@ -6,6 +6,7 @@ nullPointer:build/checkother.cpp missingOverride noValidConfiguration useStlAlgorithm +shadowLocal *:gui/test/* *:test/test.cxx diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 467552abb..3de60d15e 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2937,3 +2937,47 @@ void CheckOther::funcArgOrderDifferent(const std::string & functionName, reportError(tokens, Severity::warning, "funcArgOrderDifferent", msg, CWE683, false); } +static const Token *findShadowed(const Scope *scope, const std::string &varname, int linenr) +{ + if (!scope) + return nullptr; + for (const Variable &var : scope->varlist) { + if (scope->isExecutable() && var.nameToken()->linenr() > linenr) + continue; + if (var.name() == varname) + return var.nameToken(); + } + for (const Function &f : scope->functionList) { + if (f.name() == varname) + return f.tokenDef; + } + return findShadowed(scope->nestedIn, varname, linenr); +} + +void CheckOther::checkShadowVariables() +{ + if (!mSettings->isEnabled(Settings::STYLE)) + return; + const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); + for (const Scope & scope : symbolDatabase->scopeList) { + if (!scope.isExecutable()) + continue; + for (const Variable &var : scope.varlist) { + const Token *shadowed = findShadowed(scope.nestedIn, var.name(), var.nameToken()->linenr()); + if (!shadowed) + continue; + if (scope.type == Scope::eFunction && scope.className == var.name()) + continue; + shadowVariablesError(var.nameToken(), shadowed); + } + } +} + +void CheckOther::shadowVariablesError(const Token *var, const Token *shadowed) +{ + ErrorPath errorPath; + errorPath.push_back(ErrorPathItem(shadowed, "Shadowed declaration")); + errorPath.push_back(ErrorPathItem(var, "Shadow variable")); + const std::string &varname = var ? var->str() : "var"; + reportError(errorPath, Severity::style, "shadowLocal", "$symbol:" + varname + "\nLocal variable $symbol shadows outer symbol", CWE398, false); +} diff --git a/lib/checkother.h b/lib/checkother.h index fd91ebcab..9883359de 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -81,6 +81,7 @@ public: checkOther.checkUnusedLabel(); checkOther.checkEvaluationOrder(); checkOther.checkFuncArgNamesDifferent(); + checkOther.checkShadowVariables(); } /** @brief Run checks against the simplified token list */ @@ -211,6 +212,9 @@ public: /** @brief %Check if function declaration and definition argument names different */ void checkFuncArgNamesDifferent(); + /** @brief %Check for shadow variables. Less noisy than gcc/clang -Wshadow. */ + void checkShadowVariables(); + private: // Error messages.. void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &functionName, const std::string &varName, const bool result); @@ -263,6 +267,7 @@ private: void accessMovedError(const Token *tok, const std::string &varname, const ValueFlow::Value *value, bool inconclusive); void funcArgNamesDifferent(const std::string & functionName, size_t index, const Token* declaration, const Token* definition); void funcArgOrderDifferent(const std::string & functionName, const Token * declaration, const Token * definition, const std::vector & declarations, const std::vector & definitions); + void shadowVariablesError(const Token *var, const Token *shadowed); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const override { CheckOther c(nullptr, settings, errorLogger); @@ -324,6 +329,7 @@ private: c.accessMovedError(nullptr, "v", nullptr, false); c.funcArgNamesDifferent("function", 1, nullptr, nullptr); c.redundantBitwiseOperationInSwitchError(nullptr, "varname"); + c.shadowVariablesError(nullptr, nullptr); const std::vector nullvec; c.funcArgOrderDifferent("function", nullptr, nullptr, nullvec, nullvec); @@ -385,7 +391,8 @@ private: "- redundant pointer operation on pointer like &\\*some_ptr.\n" "- find unused 'goto' labels.\n" "- function declaration and definition argument names different.\n" - "- function declaration and definition argument order different.\n"; + "- function declaration and definition argument order different.\n" + "- shadow variable.\n"; } }; /// @} diff --git a/test/testother.cpp b/test/testother.cpp index 56025d7d4..49c8e5a41 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -214,6 +214,8 @@ private: TEST_CASE(funcArgNamesDifferent); TEST_CASE(funcArgOrderDifferent); TEST_CASE(cpp11FunctionArgInit); // #7846 - "void foo(int declaration = {}) {" + + TEST_CASE(shadowLocal); } void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, Settings* settings = 0) { @@ -2990,11 +2992,11 @@ private: // #6406 - designated initializer doing bogus self assignment check("struct callbacks {\n" - " void (*something)(void);\n" + " void (*s)(void);\n" "};\n" "void something(void) {}\n" "void f() {\n" - " struct callbacks ops = { .something = ops.something };\n" + " struct callbacks ops = { .s = ops.s };\n" "}\n"); TODO_ASSERT_EQUALS("[test.cpp:6]: (warning) Redundant assignment of 'something' to itself.\n", "", errout.str()); @@ -4621,7 +4623,7 @@ private: check("template \n" "T f() {\n" - " T a = T();\n" + " T x = T();\n" "}\n" "int &a = f();\n"); ASSERT_EQUALS("", errout.str()); @@ -5340,7 +5342,7 @@ private: " const int a = getA + 3;\n" " return 0;\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:4]: (style) Local variable getA shadows outer symbol\n", errout.str()); check("class A{public:A(){}};\n" "const A& getA(){static A a;return a;}\n" @@ -7384,6 +7386,27 @@ private: )); ASSERT_EQUALS("", errout.str()); } + + void shadowLocal() { + check("int x;\n" + "void f() { int x; }\n"); + ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2]: (style) Local variable x shadows outer symbol\n", errout.str()); + + check("int x();\n" + "void f() { int x; }\n"); + ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2]: (style) Local variable x shadows outer symbol\n", errout.str()); + + check("void f() {\n" + " if (cond) {int x;}\n" // <- not a shadow variable + " int x;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("int size() {\n" + " int size;\n" // <- not a shadow variable + "}\n"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestOther)