Added missingOverride checker; Function 'f' overrides function in base class but does not have the 'override' keyword.

lib/checkclass.cpp

@@ -2484,3 +2484,39 @@ void CheckClass::unsafeClassDivZeroError(const Token *tok, const std::string &cl
     const std::string s = className + "::" + methodName + "()";
     reportError(tok, Severity::style, "unsafeClassDivZero", symbols + "Public interface of " + className + " is not safe. When calling " + s + ", if parameter " + varName + " is 0 that leads to division by zero.");
 }
+
+void CheckClass::checkOverride()
+{
+    if (!_settings->isEnabled(Settings::STYLE))
+        return;
+    if (_settings->standards.cpp < Standards::CPP11)
+        return;
+    for (const Scope * classScope : symbolDatabase->classAndStructScopes) {
+        if (!classScope->definedType || classScope->definedType->derivedFrom.empty())
+            continue;
+        for (const Function &func : classScope->functionList) {
+            if (func.hasOverrideKeyword())
+                continue;
+            const Function *baseFunc = func.getOverridenFunction();
+            if (baseFunc)
+                overrideError(baseFunc, &func);
+        }
+    }
+}
+
+void CheckClass::overrideError(const Function *funcInBase, const Function *funcInDerived)
+{
+    const std::string functionName = funcInDerived ? funcInDerived->name() : "";
+
+    ErrorPath errorPath;
+    if (funcInBase && funcInDerived) {
+        errorPath.push_back(ErrorPathItem(funcInBase->tokenDef, "Virtual function in base class"));
+        errorPath.push_back(ErrorPathItem(funcInDerived->tokenDef, "Function in derived class"));
+    }
+
+    reportError(errorPath, Severity::style, "missingOverride",
+                "$symbol:" + functionName + "\n"
+                "Function '$symbol' overrides function in base class but does not have the 'override' keyword.",
+                CWE(0U) /* Unknown CWE! */,
+                false);
+}

lib/checkclass.h

@@ -90,6 +90,8 @@ public:
         checkClass.checkDuplInheritedMembers();
         checkClass.checkExplicitConstructors();
         checkClass.checkCopyCtorAndEqOperator();
+
+        checkClass.checkOverride();
     }
 
 
@@ -155,6 +157,9 @@ public:
     /** @brief Check that arbitrary usage of the public interface does not result in division by zero */
     void checkUnsafeClassDivZero(bool test=false);
 
+    /** @brief Check that the override keyword is used when overriding virtual methods */
+    void checkOverride();
+
 private:
     const SymbolDatabase *symbolDatabase;
 
@@ -189,6 +194,7 @@ private:
     void duplInheritedMembersError(const Token* tok1, const Token* tok2, const std::string &derivedname, const std::string &basename, const std::string &variablename, bool derivedIsStruct, bool baseIsStruct);
     void copyCtorAndEqOperatorError(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor);
     void unsafeClassDivZeroError(const Token *tok, const std::string &className, const std::string &methodName, const std::string &varName);
+    void overrideError(const Function *funcInBase, const Function *funcInDerived);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
         CheckClass c(nullptr, settings, errorLogger);
@@ -222,6 +228,7 @@ private:
         c.unsafeClassDivZeroError(nullptr, "Class", "dostuff", "x");
         c.pureVirtualFunctionCallInConstructorError(nullptr, std::list<const Token *>(), "f");
         c.virtualFunctionCallInConstructorError(nullptr, std::list<const Token *>(), "f");
+        c.overrideError(nullptr, nullptr);
     }
 
     static std::string myName() {
@@ -249,7 +256,8 @@ private:
                "- Call of pure virtual function in constructor/destructor\n"
                "- Duplicated inherited data members\n"
                "- If 'copy constructor' defined, 'operator=' also should be defined and vice versa\n"
-               "- Check that arbitrary usage of public interface does not result in division by zero\n";
+               "- Check that arbitrary usage of public interface does not result in division by zero\n"
+               "- Check that the 'override' keyword is used when overriding virtual methods\n";
     }
 
     // operatorEqRetRefThis helper functions

lib/symboldatabase.cpp

@@ -3061,74 +3061,75 @@ bool Function::isImplicitlyVirtual(bool defaultVal) const
 {
     if (isVirtual())
         return true;
-    return isOverride(defaultVal);
+    bool foundAllBaseClasses = true;
-}
+    if (getOverridenFunction(&foundAllBaseClasses))
-
-bool Function::isOverride(bool defaultVal) const
-{
-    if (!nestedIn->isClassOrStruct())
-        return false;
-    bool safe = true;
-    bool hasVirt = isOverrideRecursive(nestedIn->definedType, safe);
-    if (hasVirt)
         return true;
-    else if (safe)
+    if (foundAllBaseClasses)
         return false;
-    else
+    return defaultVal;
-        return defaultVal;
 }
 
-bool Function::isOverrideRecursive(const ::Type* baseType, bool& safe) const
+const Function *Function::getOverridenFunction(bool *foundAllBaseClasses) const
+{
+    if (foundAllBaseClasses)
+        *foundAllBaseClasses = true;
+    if (!nestedIn->isClassOrStruct())
+        return nullptr;
+    return getOverridenFunctionRecursive(nestedIn->definedType, foundAllBaseClasses);
+}
+
+const Function * Function::getOverridenFunctionRecursive(const ::Type* baseType, bool *foundAllBaseClasses) const
 {
     // check each base class
     for (std::size_t i = 0; i < baseType->derivedFrom.size(); ++i) {
         const ::Type* derivedFromType = baseType->derivedFrom[i].type;
         // check if base class exists in database
-        if (derivedFromType && derivedFromType->classScope) {
+        if (!derivedFromType || !derivedFromType->classScope) {
-            const Scope *parent = derivedFromType->classScope;
+            if (foundAllBaseClasses)
+                *foundAllBaseClasses = false;
+            continue;
+        }
 
-            // check if function defined in base class
+        const Scope *parent = derivedFromType->classScope;
-            for (std::multimap<std::string, const Function *>::const_iterator it = parent->functionMap.find(tokenDef->str()); it != parent->functionMap.end() && it->first == tokenDef->str(); ++it) {
-                const Function * func = it->second;
-                if (func->isVirtual()) { // Base is virtual and of same name
-                    const Token *temp1 = func->tokenDef->previous();
-                    const Token *temp2 = tokenDef->previous();
-                    bool returnMatch = true;
 
-                    // check for matching return parameters
+        // check if function defined in base class
-                    while (temp1->str() != "virtual") {
+        for (std::multimap<std::string, const Function *>::const_iterator it = parent->functionMap.find(tokenDef->str()); it != parent->functionMap.end() && it->first == tokenDef->str(); ++it) {
-                        if (temp1->str() != temp2->str() &&
+            const Function * func = it->second;
-                            !(temp1->str() == derivedFromType->name() &&
+            if (func->isVirtual()) { // Base is virtual and of same name
-                              temp2->str() == baseType->name())) {
+                const Token *temp1 = func->tokenDef->previous();
-                            returnMatch = false;
+                const Token *temp2 = tokenDef->previous();
-                            break;
+                bool match = true;
-                        }
 
-                        temp1 = temp1->previous();
+                // check for matching return parameters
-                        temp2 = temp2->previous();
+                while (temp1->str() != "virtual") {
+                    if (temp1->str() != temp2->str() &&
+                        !(temp1->str() == derivedFromType->name() &&
+                          temp2->str() == baseType->name())) {
+                        match = false;
+                        break;
                     }
 
-                    // check for matching function parameters
+                    temp1 = temp1->previous();
-                    if (returnMatch && argsMatch(baseType->classScope, func->argDef, argDef, emptyString, 0)) {
+                    temp2 = temp2->previous();
-                        return true;
-                    }
                 }
-            }
 
-            if (!derivedFromType->derivedFrom.empty() && !derivedFromType->hasCircularDependencies()) {
+                // check for matching function parameters
-                // avoid endless recursion, see #5289 Crash: Stack overflow in isImplicitlyVirtual_rec when checking SVN and
+                if (match && argsMatch(baseType->classScope, func->argDef, argDef, emptyString, 0)) {
-                // #5590 with a loop within the class hierarchy.
+                    return func;
-                if (isOverrideRecursive(derivedFromType, safe))  {
-                    return true;
                 }
             }
-        } else {
+        }
-            // unable to find base class so assume it has no virtual function
+
-            safe = false;
+        if (!derivedFromType->derivedFrom.empty() && !derivedFromType->hasCircularDependencies()) {
-            return false;
+            // avoid endless recursion, see #5289 Crash: Stack overflow in isImplicitlyVirtual_rec when checking SVN and
+            // #5590 with a loop within the class hierarchy.
+            const Function *func = getOverridenFunctionRecursive(derivedFromType, foundAllBaseClasses);
+            if (func)  {
+                return func;
+            }
         }
     }
-    return false;
+    return nullptr;
 }
 
 const Variable* Function::getArgumentVar(std::size_t num) const

lib/symboldatabase.h

@@ -653,25 +653,25 @@ private:
 class CPPCHECKLIB Function {
     /** @brief flags mask used to access specific bit. */
     enum {
-        fHasBody        = (1 << 0),  ///< @brief has implementation
+        fHasBody             = (1 << 0),  ///< @brief has implementation
-        fIsInline       = (1 << 1),  ///< @brief implementation in class definition
+        fIsInline            = (1 << 1),  ///< @brief implementation in class definition
-        fIsConst        = (1 << 2),  ///< @brief is const
+        fIsConst             = (1 << 2),  ///< @brief is const
-        fIsVirtual      = (1 << 3),  ///< @brief is virtual
+        fIsVirtual           = (1 << 3),  ///< @brief is virtual
-        fIsPure         = (1 << 4),  ///< @brief is pure virtual
+        fIsPure              = (1 << 4),  ///< @brief is pure virtual
-        fIsStatic       = (1 << 5),  ///< @brief is static
+        fIsStatic            = (1 << 5),  ///< @brief is static
-        fIsStaticLocal  = (1 << 6),  ///< @brief is static local
+        fIsStaticLocal       = (1 << 6),  ///< @brief is static local
-        fIsExtern       = (1 << 7),  ///< @brief is extern
+        fIsExtern            = (1 << 7),  ///< @brief is extern
-        fIsFriend       = (1 << 8),  ///< @brief is friend
+        fIsFriend            = (1 << 8),  ///< @brief is friend
-        fIsExplicit     = (1 << 9),  ///< @brief is explicit
+        fIsExplicit          = (1 << 9),  ///< @brief is explicit
-        fIsDefault      = (1 << 10), ///< @brief is default
+        fIsDefault           = (1 << 10), ///< @brief is default
-        fIsDelete       = (1 << 11), ///< @brief is delete
+        fIsDelete            = (1 << 11), ///< @brief is delete
-        fIsKwOverride   = (1 << 12), ///< @brief does declaration contain 'override' keyword?
+        fHasOverrideKeyword  = (1 << 12), ///< @brief does declaration contain 'override' keyword?
-        fIsNoExcept     = (1 << 13), ///< @brief is noexcept
+        fIsNoExcept          = (1 << 13), ///< @brief is noexcept
-        fIsThrow        = (1 << 14), ///< @brief is throw
+        fIsThrow             = (1 << 14), ///< @brief is throw
-        fIsOperator     = (1 << 15), ///< @brief is operator
+        fIsOperator          = (1 << 15), ///< @brief is operator
-        fHasLvalRefQual = (1 << 16), ///< @brief has & lvalue ref-qualifier
+        fHasLvalRefQual      = (1 << 16), ///< @brief has & lvalue ref-qualifier
-        fHasRvalRefQual = (1 << 17), ///< @brief has && rvalue ref-qualifier
+        fHasRvalRefQual      = (1 << 17), ///< @brief has && rvalue ref-qualifier
-        fIsVariadic     = (1 << 18)  ///< @brief is variadic
+        fIsVariadic          = (1 << 18)  ///< @brief is variadic
     };
 
     /**
@@ -729,11 +729,12 @@ public:
         return initArgCount;
     }
     void addArguments(const SymbolDatabase *symbolDatabase, const Scope *scope);
+
     /** @brief check if this function is virtual in the base classes */
     bool isImplicitlyVirtual(bool defaultVal = false) const;
 
-    /** @brief check if this function overrides a base class virtual function */
+    /** @brief get function in base class that is overridden */
-    bool isOverride(bool defaultVal = false) const;
+    const Function *getOverridenFunction(bool *foundAllBaseClasses = nullptr) const;
 
     bool isConstructor() const {
         return type==eConstructor ||
@@ -805,6 +806,9 @@ public:
     bool isThrow() const {
         return getFlag(fIsThrow);
     }
+    bool hasOverrideKeyword() const {
+        return getFlag(fHasOverrideKeyword);
+    }
     bool isOperator() const {
         return getFlag(fIsOperator);
     }
@@ -847,7 +851,7 @@ public:
 
 private:
     /** Recursively determine if this function overrides a virtual method in a base class */
-    bool isOverrideRecursive(const ::Type* baseType, bool& safe) const;
+    const Function * getOverridenFunctionRecursive(const ::Type* baseType, bool *foundAllBaseClasses) const;
 
     unsigned int flags;
 
@@ -902,7 +906,6 @@ private:
     void isVariadic(bool state) {
         setFlag(fIsVariadic, state);
     }
-
 };
 
 class CPPCHECKLIB Scope {

test/testclass.cpp

@@ -191,6 +191,8 @@ private:
         TEST_CASE(copyCtorAndEqOperator);
 
         TEST_CASE(unsafeClassDivZero);
+
+        TEST_CASE(override1);
     }
 
     void checkCopyCtorAndEqOperator(const char code[]) {
@@ -6597,6 +6599,28 @@ private:
                                 "void A::operator/(int x) { int a = 1000 / x; }");
         ASSERT_EQUALS("", errout.str());
     }
+
+    void checkOverride(const char code[]) {
+        // Clear the error log
+        errout.str("");
+        Settings settings;
+        settings.addEnabled("style");
+
+        // Tokenize..
+        Tokenizer tokenizer(&settings, this);
+        std::istringstream istr(code);
+        tokenizer.tokenize(istr, "test.cpp");
+
+        // Check..
+        CheckClass checkClass(&tokenizer, &settings, this);
+        checkClass.checkOverride();
+    }
+
+    void override1() {
+        checkOverride("class Base { virtual void f(); };\n"
+                      "class Derived : Base { virtual void f(); };");
+        ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2]: (style) Function 'f' overrides function in base class but does not have the 'override' keyword.\n", errout.str());
+    }
 };
 
 REGISTER_TEST(TestClass)