Warn when shifting a negative value, it is UB. (#4931)

lib/checkother.cpp

@@ -2177,54 +2177,53 @@ void CheckOther::redundantCopyError(const Token *tok,const std::string& varname)
 
 void CheckOther::checkNegativeBitwiseShift()
 {
-    const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
+    for (const Token* tok = _tokenizer->tokens(); tok; tok = tok->next()) {
-    const std::size_t functions = symbolDatabase->functionScopes.size();
+        if (tok->str() != "<<" && tok->str() != ">>")
-    for (std::size_t i = 0; i < functions; ++i) {
+            continue;
-        const Scope * scope = symbolDatabase->functionScopes[i];
+
-        for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) {
+        if (!tok->astOperand1() || !tok->astOperand2())
-            if (tok->str() != "<<" && tok->str() != ">>")
+            continue;
+
+        // 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())
-            if (!tok->astOperand1() || !tok->astOperand2())
                 continue;
-
+            if (rhs->variable() &&
-            // don't warn if lhs is a class. this is an overloaded operator then
+                (!rhs->variable()->typeStartToken() || !rhs->variable()->typeStartToken()->isStandardType()))
-            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()))
-                    continue;
-            }
-
-            // bailout if operation is protected by ?:
-            bool ternary = false;
-            for (const Token *parent = tok; parent; parent = parent->astParent()) {
-                if (Token::Match(parent, "?|:")) {
-                    ternary = true;
-                    break;
-                }
-            }
-            if (ternary)
                 continue;
-
-            // Get negative rhs value. preferably a value which doesn't have 'condition'.
-            const ValueFlow::Value *value = tok->astOperand2()->getValueLE(-1LL, _settings);
-            if (value)
-                negativeBitwiseShiftError(tok);
         }
+
+        // bailout if operation is protected by ?:
+        bool ternary = false;
+        for (const Token *parent = tok; parent; parent = parent->astParent()) {
+            if (Token::Match(parent, "?|:")) {
+                ternary = true;
+                break;
+            }
+        }
+        if (ternary)
+            continue;
+
+        // Get negative rhs value. preferably a value which doesn't have 'condition'.
+        if (tok->astOperand1()->getValueLE(-1LL, _settings))
+            negativeBitwiseShiftError(tok, 1);
+        else if (tok->astOperand2()->getValueLE(-1LL, _settings))
+            negativeBitwiseShiftError(tok, 2);
     }
 }
 
 
-void CheckOther::negativeBitwiseShiftError(const Token *tok)
+void CheckOther::negativeBitwiseShiftError(const Token *tok, int op)
 {
-    reportError(tok, Severity::error, "shiftNegative", "Shifting by a negative value is undefined behaviour");
+    if (op == 1) // LHS
+        reportError(tok, Severity::error, "shiftNegative", "Shifting a negative value is undefined behaviour");
+    else // RHS
+        reportError(tok, Severity::error, "shiftNegative", "Shifting by a negative value is undefined behaviour");
 }
 
 //---------------------------------------------------------------------------

lib/checkother.h

@@ -243,7 +243,7 @@ private:
     void unsignedPositiveError(const Token *tok, const std::string &varname, bool inconclusive);
     void pointerPositiveError(const Token *tok, bool inconclusive);
     void SuspiciousSemicolonError(const Token *tok);
-    void negativeBitwiseShiftError(const Token *tok);
+    void negativeBitwiseShiftError(const Token *tok, int op);
     void redundantCopyError(const Token *tok, const std::string &varname);
     void incompleteArrayFillError(const Token* tok, const std::string& buffer, const std::string& function, bool boolean);
     void varFuncNullUBError(const Token *tok);
@@ -260,7 +260,7 @@ private:
         c.zerodivcondError(0,0,false);
         c.misusedScopeObjectError(NULL, "varname");
         c.invalidPointerCastError(0, "float", "double", false);
-        c.negativeBitwiseShiftError(0);
+        c.negativeBitwiseShiftError(0,1);
         c.checkPipeParameterSizeError(0, "varname", "dimension");
         c.raceAfterInterlockedDecrementError(0);
 

test/testother.cpp

@@ -4774,6 +4774,10 @@ private:
 
         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());
     }
 
     void incompleteArrayFill() {