Fix ticket #3317 (same expression false positives)

Add a check for function calls that have no side effects. That means
known const methods and a list including strcmp, strlen, etc.

If the function is not known to be side effect-free then no style
warning is given.

Add test cases for the duplicate expressions.

lib/checkother.cpp

@@ -2371,35 +2371,141 @@ void CheckOther::duplicateBranchError(const Token *tok1, const Token *tok2)
 }
 
 namespace {
+    struct ExpressionTokens {
+        const Token *start;
+        const Token *end;
+        int count;
+        ExpressionTokens(const Token *s, const Token *e): start(s), end(e), count(1) {}
+    };
+
     class Expressions {
     public:
-        void endExpr() {
+        Expressions(): _start(0) {}
+
+        void endExpr(const Token *end) {
             const std::string &e = _expression.str();
             if (!e.empty()) {
-                std::map<std::string,int>::const_iterator it = _expressions.find(e);
+                std::map<std::string, ExpressionTokens>::iterator it = _expressions.find(e);
                 if (it == _expressions.end())
-                    _expressions[e] = 1;
+                    _expressions.insert(std::make_pair(e, ExpressionTokens(_start, end)));
                 else
-                    _expressions[e] += 1;
+                    it->second.count += 1;
             }
             _expression.str("");
+            _start = 0;
         }
 
-        void append(const std::string &tok) {
+        void append(const Token *tok) {
-            _expression << tok;
+            if (!_start)
+                _start = tok;
+            _expression << tok->str();
         }
 
-        std::map<std::string,int> &getMap() {
+        std::map<std::string,ExpressionTokens> &getMap() {
             return _expressions;
         }
 
     private:
-        std::map<std::string, int> _expressions;
+        std::map<std::string, ExpressionTokens> _expressions;
         std::ostringstream _expression;
+        const Token *_start;
     };
+
+    struct FuncFilter {
+        FuncFilter(const Scope *scope, const Token *tok): _scope(scope), _tok(tok) {}
+
+        bool operator() (const Function &func) {
+            // todo: function args, etc??
+            bool matchingFunc = func.type == Function::eFunction &&
+                                _tok->str() == func.token->str();
+            // either a class function, or a global function with the same name
+            return (_scope && _scope == func.functionScope && matchingFunc) ||
+                   (!_scope && matchingFunc);
+        }
+        const Scope *_scope;
+        const Token *_tok;
+    };
+
+
+    bool inconclusiveFunctionCall(const SymbolDatabase *symbolDatabase,
+                                  const std::list<Function> &constFunctions,
+                                  const ExpressionTokens &tokens)
+    {
+        const Token *start = tokens.start;
+        const Token *end = tokens.end;
+        // look for function calls between start and end...
+        for (const Token *tok = start; tok && tok != end; tok = tok->next()) {
+            if (tok != start && tok->str() == "(") {
+                // go back to find the function call.
+                const Token *prev = tok->previous();
+                if (prev->str() == ">") {
+                    // ignore template functions like boo<double>()
+                    return true;
+                }
+                if (prev && prev->isName()) {
+                    const Variable *v = 0;
+                    if (Token::Match(prev->tokAt(-2), "%var% .")) {
+                        const Token *scope = prev->tokAt(-2);
+                        v = symbolDatabase->getVariableFromVarId(scope->varId());
+                    }
+                    // hard coded list of safe, no-side-effect functions
+                    if (v == 0 && Token::Match(prev, "strcmp|strncmp|strlen|memcmp|strcasecmp|strncasecmp"))
+                        return false;
+                    std::list<Function>::const_iterator it = std::find_if(constFunctions.begin(),
+                                                                          constFunctions.end(),
+                                                                          FuncFilter(v ? v->type(): 0, prev));
+                    if (it == constFunctions.end())
+                        return true;
+                }
+            }
+        }
+        return false;
+    }
+
+    bool notconst(const Function &func)
+    {
+        return !func.isConst;
+    }
+
+    void getConstFunctions(const SymbolDatabase *symbolDatabase, std::list<Function> &constFunctions)
+    {
+        std::list<Scope>::const_iterator scope;
+        for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) {
+            std::list<Function>::const_iterator func;
+            // only add const functions that do not have a non-const overloaded version
+            // since it is pretty much impossible to tell which is being called.
+            typedef std::map<std::string, std::list<Function> > StringFunctionMap;
+            StringFunctionMap functionsByName;
+            for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) {
+                StringFunctionMap::iterator it = functionsByName.find(func->token->str());
+                Scope *currScope = const_cast<Scope*>(&*scope);
+                if (it == functionsByName.end()) {
+                    std::list<Function> tmp;
+                    tmp.push_back(*func);
+                    tmp.back().functionScope = currScope;
+                    functionsByName[func->token->str()] = tmp;
+                } else {
+                    it->second.push_back(*func);
+                    it->second.back().functionScope = currScope;
+                }
+            }
+            for (StringFunctionMap::iterator it = functionsByName.begin();
+                 it != functionsByName.end(); ++it) {
+                std::list<Function>::const_iterator nc = std::find_if(it->second.begin(), it->second.end(), notconst);
+                if (nc == it->second.end()) {
+                    // ok to add all of them
+                    constFunctions.splice(constFunctions.end(), it->second);
+                }
+            }
+        }
+    }
+
 }
 
-void CheckOther::checkExpressionRange(const Token *start, const Token *end, const std::string &toCheck)
+void CheckOther::checkExpressionRange(const std::list<Function> &constFunctions,
+                                      const Token *start,
+                                      const Token *end,
+                                      const std::string &toCheck)
 {
     if (!start || !end)
         return;
@@ -2414,20 +2520,25 @@ void CheckOther::checkExpressionRange(const Token *start, const Token *end, cons
 
         if (level == 0 && Token::Match(tok, toCheck.c_str())) {
             opName = tok->str();
-            expressions.endExpr();
+            expressions.endExpr(tok);
         } else {
-            expressions.append(tok->str());
+            expressions.append(tok);
         }
     }
-    expressions.endExpr();
+    expressions.endExpr(end);
-    std::map<std::string,int>::const_iterator it = expressions.getMap().begin();
+    std::map<std::string,ExpressionTokens>::const_iterator it = expressions.getMap().begin();
+    const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
     for (; it != expressions.getMap().end(); ++it) {
-        if (it->second > 1)
+        if (it->second.count > 1 &&
-            duplicateExpressionError(start, start, opName);
+            (it->first.find("(") == std::string::npos ||
+             !inconclusiveFunctionCall(symbolDatabase, constFunctions, it->second))) {
+            duplicateExpressionError(it->second.start, it->second.start, opName);
+        }
     }
 }
 
-void CheckOther::complexDuplicateExpressionCheck(const Token *classStart,
+void CheckOther::complexDuplicateExpressionCheck(const std::list<Function> &constFunctions,
+                                                 const Token *classStart,
                                                  const std::string &toCheck,
                                                  const std::string &alt)
 {
@@ -2470,11 +2581,10 @@ void CheckOther::complexDuplicateExpressionCheck(const Token *classStart,
                 break;
             }
         }
-        checkExpressionRange(start, end, toCheck);
+        checkExpressionRange(constFunctions, start, end, toCheck);
     }
 }
 
-
 //---------------------------------------------------------------------------
 // check for the same expression on both sides of an operator
 // (x == x), (x && x), (x || x)
@@ -2489,16 +2599,18 @@ void CheckOther::checkDuplicateExpression()
     const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
 
     std::list<Scope>::const_iterator scope;
+    std::list<Function> constFunctions;
+    getConstFunctions(symbolDatabase, constFunctions);
 
     for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) {
         // only check functions
         if (scope->type != Scope::eFunction)
             continue;
 
-        complexDuplicateExpressionCheck(scope->classStart, "%or%", "");
+        complexDuplicateExpressionCheck(constFunctions, scope->classStart, "%or%", "");
-        complexDuplicateExpressionCheck(scope->classStart, "%oror%", "");
+        complexDuplicateExpressionCheck(constFunctions, scope->classStart, "%oror%", "");
-        complexDuplicateExpressionCheck(scope->classStart, "&", "%oror%|%or%");
+        complexDuplicateExpressionCheck(constFunctions, scope->classStart, "&", "%oror%|%or%");
-        complexDuplicateExpressionCheck(scope->classStart, "&&", "%oror%|%or%");
+        complexDuplicateExpressionCheck(constFunctions, scope->classStart, "&&", "%oror%|%or%");
 
         for (const Token *tok = scope->classStart; tok && tok != scope->classStart->link(); tok = tok->next()) {
             if (Token::Match(tok, ",|=|return|(|&&|%oror% %var% ==|!=|<=|>=|<|>|- %var% )|&&|%oror%|;|,") &&

lib/checkother.h

@@ -26,6 +26,7 @@
 #include "settings.h"
 
 class Token;
+class Function;
 
 /// @addtogroup Checks
 /// @{
@@ -434,8 +435,13 @@ private:
         return varname;
     }
 
-    void checkExpressionRange(const Token *start, const Token *end, const std::string &toCheck);
+    void checkExpressionRange(const std::list<Function> &constFunctions,
-    void complexDuplicateExpressionCheck(const Token *classStart,
+                              const Token *start,
+                              const Token *end,
+                              const std::string &toCheck);
+
+    void complexDuplicateExpressionCheck(const std::list<Function> &constFunctions,
+                                         const Token *classStart,
                                          const std::string &toCheck,
                                          const std::string &alt);
 };

test/testother.cpp

@@ -142,6 +142,7 @@ private:
         TEST_CASE(duplicateBranch);
         TEST_CASE(duplicateExpression1);
         TEST_CASE(duplicateExpression2); // ticket #2730
+        TEST_CASE(duplicateExpression3); // ticket #3317
 
         TEST_CASE(alwaysTrueFalseStringCompare);
         TEST_CASE(checkStrncmpSizeof);
@@ -3630,11 +3631,6 @@ private:
               "}");
         ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str());
 
-        check("void foo() {\n"
-              "    if (this->bar() || this->bar()) {}\n"
-              "}");
-        ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str());
-
         check("void foo() {\n"
               "    if (a && b || a && b) {}\n"
               "}");
@@ -3689,6 +3685,64 @@ private:
                       "[test.cpp:9]: (error) Passing value -1.0 to sqrtf() leads to undefined result\n", errout.str());
     }
 
+    void duplicateExpression3() {
+        check("void foo() {\n"
+              "    if (x() || x()) {}\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("struct A {\n"
+              "  void foo() const;\n"
+              "  bool bar() const;\n"
+              "};\n"
+              "void A::foo() const {\n"
+              "    if (bar() && bar()) {}\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:6]: (style) Same expression on both sides of '&&'.\n", errout.str());
+
+        check("struct A {\n"
+              "  void foo();\n"
+              "  bool bar();\n"
+              "  bool bar() const;\n"
+              "};\n"
+              "void A::foo() {\n"
+              "    if (bar() && bar()) {}\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("class B {\n"
+              "    void bar(int i);\n"
+              "};\n"
+              "class A {\n"
+              "    void bar(int i) const;\n"
+              "};\n"
+              "void foo() {\n"
+              "    B b;\n"
+              "    A a;\n"
+              "    if (b.bar(1) && b.bar(1)) {}\n"
+              "    if (a.bar(1) && a.bar(1)) {}\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:11] -> [test.cpp:11]: (style) Same expression on both sides of '&&'.\n", errout.str());
+
+        check("class D { void strcmp(); };\n"
+              "void foo() {\n"
+              "    D d;\n"
+              "    if (d.strcmp() && d.strcmp()) {}\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void foo() {\n"
+              "    if ((strcmp(a, b) == 0) || (strcmp(a, b) == 0)) {}\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str());
+
+        check("void foo() {\n"
+              "    if (str == \"(\" || str == \"(\") {}\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str());
+    }
+
+
     void alwaysTrueFalseStringCompare() {
         check_preprocess_suppress(
             "#define MACRO \"00FF00\"\n"