STL: suspicious condition when using find on strings and containers

lib/checkstl.cpp

@@ -531,6 +531,44 @@ void CheckStl::findError(const Token *tok)
     reportError(tok, Severity::error, "stlfind", "dangerous usage of find result");
 }
 
+
+
+void CheckStl::if_find()
+{
+    for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next())
+    {
+        if (Token::Match(tok, "if ( !| %var% . find ( %any% ) )"))
+        {
+            // goto %var%
+            tok = tok->tokAt(2);
+            if (!tok->isName())
+                tok = tok->next();
+
+            const unsigned int varid = tok->varId();
+            if (varid > 0)
+            {
+                // Locate variable declaration..
+                const Token * const decl = Token::findmatch(_tokenizer->tokens(), "%varid%", varid);
+                if (Token::Match(decl->tokAt(-4), ",|;|( std :: string"))
+                    if_findError(tok, true);
+                else if (Token::Match(decl->tokAt(-7), ",|;|( std :: %type% < %type% >"))
+                    if_findError(tok, false);
+            }
+        }
+    }
+}
+
+
+void CheckStl::if_findError(const Token *tok, bool str)
+{
+    if (str)
+        reportError(tok, Severity::possibleStyle, "stlIfStrFind", "Suspicious condition. string::find will return 0 if the string is found at position 0. If this is what you want to check then string::compare is a faster alternative because it doesn't scan through the string.");
+    else
+        reportError(tok, Severity::style, "stlIfFind", "Suspicious condition. The result of find is an iterator, but it is not properly checked.");
+}
+
+
+
 bool CheckStl::isStlContainer(const Token *tok)
 {
     // check if this token is defined

lib/checkstl.h

@@ -54,6 +54,7 @@ public:
         checkStl.pushback();
         checkStl.stlBoundries();
         checkStl.find();
+        checkStl.if_find();
 
         if (settings->_checkCodingStyle)
         {
@@ -99,9 +100,12 @@ public:
      */
     void stlBoundries();
 
-    /** usage of std::find */
+    /** usage of std::find - proper handling of return iterator*/
     void find();
 
+    /** if (a.find(x)) - possibly incorrect condition */
+    void if_find();
+
     /**
      * Suggest using empty() instead of checking size() against zero for containers.
      * Item 4 from Scott Meyers book "Effective STL".
@@ -125,6 +129,7 @@ private:
     void invalidPointerError(const Token *tok, const std::string &pointer_name);
     void stlBoundriesError(const Token *tok, const std::string &container_name);
     void findError(const Token *tok);
+    void if_findError(const Token *tok, bool str);
     void sizeError(const Token *tok);
 
     void getErrorMessages()
@@ -138,6 +143,8 @@ private:
         invalidPointerError(0, "pointer");
         stlBoundriesError(0, "container");
         findError(0);
+        if_findError(0, false);
+        if_findError(0, true);
         sizeError(0);
     }
 
@@ -155,7 +162,8 @@ private:
                "* dereferencing an erased iterator\n"
                "* for vectors: using iterator/pointer after push_back has been used\n"
                "* dangerous usage of find\n"
-               "* optimisation: use empty() instead of size() to guarantee fast code\n";
+               "* optimisation: use empty() instead of size() to guarantee fast code\n"
+               "* suspicious condition when using find\n";
     }
 
     bool isStlContainer(const Token *tok);

test/teststl.cpp

@@ -72,6 +72,9 @@ private:
         TEST_CASE(stlBoundries2);
         TEST_CASE(stlBoundries3);
 
+        // if (str.find("ab"))
+        TEST_CASE(if_find);
+
         // find
         TEST_CASE(find1);
 
@@ -640,6 +643,24 @@ private:
     }
 
 
+
+    void if_find()
+    {
+        check("void f(std::string s)\n"
+              "{\n"
+              "    if (s.find(\"ab\")) { }\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (possible style) Suspicious condition. string::find will return 0 if the string is found at position 0. If this is what you want to check then string::compare is a faster alternative because it doesn't scan through the string.\n", errout.str());
+
+        check("void f(std::set<int> s)\n"
+              "{\n"
+              "    if (s.find(12)) { }\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Suspicious condition. The result of find is an iterator, but it is not properly checked.\n", errout.str());
+    }
+
+
+
     void find1()
     {
         check("void f(std::vector<int> &ints)\n"