From eb288ec2a148bf3ff9d3792e3ca54a256a95f803 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= <daniel.marjamaki@gmail.com>
Date: Wed, 26 Jul 2017 23:13:01 +0200
Subject: [PATCH] CheckStl: Use AST to handle iterator comparisons better

---
 lib/checkstl.cpp | 12 +++++++++---
 test/teststl.cpp | 19 ++++++++++++++++++-
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp
index 54741e738..ae265271d 100644
--- a/lib/checkstl.cpp
+++ b/lib/checkstl.cpp
@@ -140,9 +140,15 @@ void CheckStl::iterators()
                 validIterator = true;
 
             // Is iterator compared against different container?
-            if (Token::Match(tok2, "%varid% !=|== %name% . end|rend|cend|crend ( )", iteratorId) && container && tok2->tokAt(2)->varId() != container->declarationId()) {
-                iteratorsError(tok2, container->name(), tok2->strAt(2));
-                tok2 = tok2->tokAt(6);
+            if (tok2->isComparisonOp() && container) {
+                const Token *other = nullptr;
+                if (tok2->astOperand1()->varId() == iteratorId)
+                    other = tok2->astOperand2()->tokAt(-3);
+                else if (tok2->astOperand2()->varId() == iteratorId)
+                    other = tok2->astOperand1()->tokAt(-3);
+
+                if (Token::Match(other, "%name% . end|rend|cend|crend ( )") && other->varId() != container->declarationId())
+                    iteratorsError(tok2, container->name(), other->str());
             }
 
             // Is the iterator used in a insert/erase operation?
diff --git a/test/teststl.cpp b/test/teststl.cpp
index a4889178c..ccb1ca348 100644
--- a/test/teststl.cpp
+++ b/test/teststl.cpp
@@ -172,6 +172,15 @@ private:
               "}");
         ASSERT_EQUALS("[test.cpp:5]: (error) Same iterator is used with different containers 'l1' and 'l2'.\n", errout.str());
 
+        check("void f()\n"
+              "{\n"
+              "    list<int> l1;\n"
+              "    list<int> l2;\n"
+              "    for (list<int>::iterator it = l1.begin(); l2.end() != it; ++it)\n"
+              "    { }\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Same iterator is used with different containers 'l1' and 'l2'.\n", errout.str());
+
         // Same check with reverse iterator
         check("void f()\n"
               "{\n"
@@ -442,6 +451,14 @@ private:
               "}");
         ASSERT_EQUALS("[test.cpp:5]: (error) Same iterator is used with different containers 'map1' and 'map2'.\n", errout.str());
 
+        check("void f() {\n"
+              "    std::map<int, int> map1;\n"
+              "    std::map<int, int> map2;\n"
+              "    std::map<int, int>::const_iterator it = map1.find(123);\n"
+              "    if (map2.end() == it) { }"
+              "}");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Same iterator is used with different containers 'map1' and 'map2'.\n", errout.str());
+
         check("void f(std::string &s) {\n"
               "    int pos = s.find(x);\n"
               "    s.erase(pos);\n"
@@ -457,7 +474,7 @@ private:
               "    std::vector<int>::const_iterator it;\n"
               "    it = a.begin();\n"
               "    while (it!=a.end())\n"
-              "        v++it;\n"
+              "        ++it;\n"
               "    it = t.begin();\n"
               "    while (it!=a.end())\n"
               "        ++it;\n"