From df6ae9f3b43fbf2febfc981684f80b288607d02c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= <daniel.marjamaki@gmail.com>
Date: Thu, 22 Dec 2016 09:40:19 +0100
Subject: [PATCH] Fixed #7847 (Can't detect shift negative values when some op
 is executed)

---
 lib/checkother.cpp | 22 ++++++++++------------
 lib/checkother.h   |  1 +
 test/testother.cpp | 10 +++++++---
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/lib/checkother.cpp b/lib/checkother.cpp
index bc78a9c83..8b9aa9d18 100644
--- a/lib/checkother.cpp
+++ b/lib/checkother.cpp
@@ -2303,6 +2303,8 @@ static bool isNegative(const Token *tok, const Settings *settings)
 
 void CheckOther::checkNegativeBitwiseShift()
 {
+    const bool portability = _settings->isEnabled("portability");
+
     for (const Token* tok = _tokenizer->tokens(); tok; tok = tok->next()) {
         if (!tok->astOperand1() || !tok->astOperand2())
             continue;
@@ -2312,15 +2314,8 @@ void CheckOther::checkNegativeBitwiseShift()
 
         // don't warn if lhs is a class. this is an overloaded operator then
         if (_tokenizer->isCPP()) {
-            const Token *rhs = tok->astOperand1();
-            while (Token::Match(rhs, "::|."))
-                rhs = rhs->astOperand2();
-            if (!rhs)
-                continue;
-            if (!rhs->isNumber() && !rhs->variable())
-                continue;
-            if (rhs->variable() &&
-                (!rhs->variable()->typeStartToken() || !rhs->variable()->typeStartToken()->isStandardType()))
+            const ValueType * lhsType = tok->astOperand1()->valueType();
+            if (!lhsType || !lhsType->isIntegral())
                 continue;
         }
 
@@ -2336,7 +2331,7 @@ void CheckOther::checkNegativeBitwiseShift()
             continue;
 
         // Get negative rhs value. preferably a value which doesn't have 'condition'.
-        if (isNegative(tok->astOperand1(), _settings))
+        if (portability && isNegative(tok->astOperand1(), _settings))
             negativeBitwiseShiftError(tok, 1);
         else if (isNegative(tok->astOperand2(), _settings))
             negativeBitwiseShiftError(tok, 2);
@@ -2346,8 +2341,11 @@ void CheckOther::checkNegativeBitwiseShift()
 
 void CheckOther::negativeBitwiseShiftError(const Token *tok, int op)
 {
-    if (op == 1) // LHS
-        reportError(tok, Severity::error, "shiftNegative", "Shifting a negative value is undefined behaviour", CWE758, false);
+    if (op == 1)
+        // LHS - this is used by intention in various software, if it
+        // is used often in a project and works as expected then this is
+        // a portability issue
+        reportError(tok, Severity::portability, "shiftNegativeLHS", "Shifting a negative value is technically undefined behaviour", CWE758, false);
     else // RHS
         reportError(tok, Severity::error, "shiftNegative", "Shifting by a negative value is undefined behaviour", CWE758, false);
 }
diff --git a/lib/checkother.h b/lib/checkother.h
index 8cc76e7a7..5a624630c 100644
--- a/lib/checkother.h
+++ b/lib/checkother.h
@@ -268,6 +268,7 @@ private:
         c.misusedScopeObjectError(nullptr, "varname");
         c.invalidPointerCastError(nullptr,  "float", "double", false);
         c.negativeBitwiseShiftError(nullptr, 1);
+        c.negativeBitwiseShiftError(nullptr, 2);
         c.checkPipeParameterSizeError(nullptr,  "varname", "dimension");
         c.raceAfterInterlockedDecrementError(nullptr);
 
diff --git a/test/testother.cpp b/test/testother.cpp
index e029e899c..52bc145ad 100644
--- a/test/testother.cpp
+++ b/test/testother.cpp
@@ -4618,13 +4618,17 @@ private:
               "   std::cout << 3 << -1 ;\n"
               "}");
         ASSERT_EQUALS("", errout.str());
+        check("void foo() {\n"
+              "   x = (-10+2) << 3;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (portability) Shifting a negative value is technically undefined behaviour\n", errout.str());
 
         check("x = y ? z << $-1 : 0;\n");
         ASSERT_EQUALS("", errout.str());
 
         // Negative LHS
         check("const int x = -1 >> 2;");
-        ASSERT_EQUALS("[test.cpp:1]: (error) Shifting a negative value is undefined behaviour\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:1]: (portability) Shifting a negative value is technically undefined behaviour\n", errout.str());
 
         // #6383 - unsigned type
         check("const int x = (unsigned int)(-1) >> 2;");
@@ -4637,8 +4641,8 @@ private:
               "int shift4() { return -1 << 1 ;}\n");
         ASSERT_EQUALS("[test.cpp:1]: (error) Shifting by a negative value is undefined behaviour\n"
                       "[test.cpp:2]: (error) Shifting by a negative value is undefined behaviour\n"
-                      "[test.cpp:3]: (error) Shifting a negative value is undefined behaviour\n"
-                      "[test.cpp:4]: (error) Shifting a negative value is undefined behaviour\n", errout.str());
+                      "[test.cpp:3]: (portability) Shifting a negative value is technically undefined behaviour\n"
+                      "[test.cpp:4]: (portability) Shifting a negative value is technically undefined behaviour\n", errout.str());
     }
 
     void incompleteArrayFill() {