Fix #12431 (Style warnings are reported when only --enable=warning is used) (#5974)

* do not show knownConditionTrueFalse, virtualCallInConstructor, duplicateExpression style messages unless --enable=style is configured.
* change selfAssignment from warning to style => it's not about undefined behavior
* don't claim that code such as `a = b|c;`  is a condition.

Author: danmar

lib/checkclass.cpp

@@ -2891,6 +2891,9 @@ void CheckClass::virtualFunctionCallInConstructorError(
     const std::list<const Token *> & tokStack,
     const std::string &funcname)
 {
+    if (scopeFunction && !mSettings->severity.isEnabled(Severity::style) && !mSettings->isPremiumEnabled("virtualCallInConstructor"))
+        return;
+
     const char * scopeFunctionTypeName = scopeFunction ? getFunctionTypeName(scopeFunction->type) : "constructor";
 
     ErrorPath errorPath;

lib/checkother.cpp

@@ -2402,10 +2402,19 @@ namespace {
 
 void CheckOther::checkDuplicateExpression()
 {
-    const bool styleEnabled = mSettings->severity.isEnabled(Severity::style);
-    const bool warningEnabled = mSettings->severity.isEnabled(Severity::warning);
-    if (!styleEnabled && !warningEnabled)
-        return;
+    {
+        const bool styleEnabled = mSettings->severity.isEnabled(Severity::style);
+        const bool premiumEnabled = mSettings->isPremiumEnabled("oppositeExpression") ||
+                                    mSettings->isPremiumEnabled("duplicateExpression") ||
+                                    mSettings->isPremiumEnabled("duplicateAssignExpression") ||
+                                    mSettings->isPremiumEnabled("duplicateExpressionTernary") ||
+                                    mSettings->isPremiumEnabled("duplicateValueTernary") ||
+                                    mSettings->isPremiumEnabled("selfAssignment") ||
+                                    mSettings->isPremiumEnabled("knownConditionTrueFalse");
+
+        if (!styleEnabled && !premiumEnabled)
+            return;
+    }
 
     logChecker("CheckOther::checkDuplicateExpression"); // style,warning
 
@@ -2511,9 +2520,9 @@ void CheckOther::checkDuplicateExpression()
                             !findExpressionChanged(tok, tok, loopTok->link()->next()->link(), mSettings, cpp)) {
                             const bool isEnum = tok->scope()->type == Scope::eEnum;
                             const bool assignment = !isEnum && tok->str() == "=";
-                            if (assignment && warningEnabled)
+                            if (assignment)
                                 selfAssignmentError(tok, tok->astOperand1()->expressionString());
-                            else if (styleEnabled && !isEnum) {
+                            else if (!isEnum) {
                                 if (cpp && mSettings->standards.cpp >= Standards::CPP11 && tok->str() == "==") {
                                     const Token* parent = tok->astParent();
                                     while (parent && parent->astParent()) {
@@ -2535,11 +2544,10 @@ void CheckOther::checkDuplicateExpression()
                                             mSettings->library,
                                             true,
                                             false)) {
-                    if (warningEnabled && isWithoutSideEffects(cpp, tok->astOperand1())) {
+                    if (isWithoutSideEffects(cpp, tok->astOperand1())) {
                         selfAssignmentError(tok, tok->astOperand1()->expressionString());
                     }
-                } else if (styleEnabled &&
-                           isOppositeExpression(cpp,
+                } else if (isOppositeExpression(cpp,
                                                 tok->astOperand1(),
                                                 tok->astOperand2(),
                                                 mSettings->library,
@@ -2550,7 +2558,7 @@ void CheckOther::checkDuplicateExpression()
                            isWithoutSideEffects(cpp, tok->astOperand1())) {
                     oppositeExpressionError(tok, std::move(errorPath));
                 } else if (!Token::Match(tok, "[-/%]")) { // These operators are not associative
-                    if (styleEnabled && tok->astOperand2() && tok->str() == tok->astOperand1()->str() &&
+                    if (tok->astOperand2() && tok->str() == tok->astOperand1()->str() &&
                         isSameExpression(cpp,
                                          true,
                                          tok->astOperand2(),
@@ -2577,7 +2585,7 @@ void CheckOther::checkDuplicateExpression()
                         }
                     }
                 }
-            } else if (styleEnabled && tok->astOperand1() && tok->astOperand2() && tok->str() == ":" && tok->astParent() && tok->astParent()->str() == "?") {
+            } else if (tok->astOperand1() && tok->astOperand2() && tok->str() == ":" && tok->astParent() && tok->astParent()->str() == "?") {
                 if (!tok->astOperand1()->values().empty() && !tok->astOperand2()->values().empty() && isEqualKnownValue(tok->astOperand1(), tok->astOperand2()) &&
                     !isVariableChanged(tok->astParent(), /*indirect*/ 0, mSettings, cpp) &&
                     isConstStatement(tok->astOperand1(), cpp) && isConstStatement(tok->astOperand2(), cpp))
@@ -2611,17 +2619,18 @@ void CheckOther::duplicateExpressionError(const Token *tok1, const Token *tok2,
     const std::string& op = opTok ? opTok->str() : "&&";
     std::string msg = "Same expression " + (hasMultipleExpr ? "\'" + expr1 + "\'" + " found multiple times in chain of \'" + op + "\' operators" : "on both sides of \'" + op + "\'");
     const char *id = "duplicateExpression";
-    if (expr1 != expr2 && (!opTok || !opTok->isArithmeticalOp())) {
+    if (expr1 != expr2 && (!opTok || Token::Match(opTok, "%oror%|%comp%|&&|?|!"))) {
         id = "knownConditionTrueFalse";
         std::string exprMsg = "The comparison \'" + expr1 + " " + op +  " " + expr2 + "\' is always ";
         if (Token::Match(opTok, "==|>=|<="))
             msg = exprMsg + "true";
         else if (Token::Match(opTok, "!=|>|<"))
             msg = exprMsg + "false";
-        if (!Token::Match(tok1, "%num%|NULL|nullptr") && !Token::Match(tok2, "%num%|NULL|nullptr"))
-            msg += " because '" + expr1 + "' and '" + expr2 + "' represent the same value";
     }
 
+    if (expr1 != expr2 && !Token::Match(tok1, "%num%|NULL|nullptr") && !Token::Match(tok2, "%num%|NULL|nullptr"))
+        msg += " because '" + expr1 + "' and '" + expr2 + "' represent the same value";
+
     reportError(errors, Severity::style, id, msg +
                 (std::string(".\nFinding the same expression ") + (hasMultipleExpr ? "more than once in a condition" : "on both sides of an operator")) +
                 " is suspicious and might indicate a cut and paste or logic error. Please examine this code carefully to "
@@ -2659,7 +2668,7 @@ void CheckOther::duplicateValueTernaryError(const Token *tok)
 
 void CheckOther::selfAssignmentError(const Token *tok, const std::string &varname)
 {
-    reportError(tok, Severity::warning,
+    reportError(tok, Severity::style,
                 "selfAssignment",
                 "$symbol:" + varname + "\n"
                 "Redundant assignment of '$symbol' to itself.", CWE398, Certainty::normal);

test/testclass.cpp

@@ -8027,7 +8027,7 @@ class TestClass : public TestFixture {
         errout.str("");
 
         // Check..
-        const Settings settings = settingsBuilder().severity(Severity::warning).certainty(Certainty::inconclusive, inconclusive).build();
+        const Settings settings = settingsBuilder().severity(Severity::warning).severity(Severity::style).certainty(Certainty::inconclusive, inconclusive).build();
 
         Preprocessor preprocessor(settings);
 

test/testother.cpp

@@ -5467,26 +5467,26 @@ class TestOther : public TestFixture {
               "    x = x;\n"
               "    return 0;\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:4]: (warning) Redundant assignment of 'x' to itself.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:4]: (style) Redundant assignment of 'x' to itself.\n", errout.str());
 
         check("void foo()\n"
               "{\n"
               "    int x = x;\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'x' to itself.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:3]: (style) Redundant assignment of 'x' to itself.\n", errout.str());
 
         check("struct A { int b; };\n"
               "void foo(A* a1, A* a2) {\n"
               "    a1->b = a1->b;\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'a1->b' to itself.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:3]: (style) Redundant assignment of 'a1->b' to itself.\n", errout.str());
 
         check("int x;\n"
               "void f()\n"
               "{\n"
               "    x = x = 3;\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:4]: (warning) Redundant assignment of 'x' to itself.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:4]: (style) Redundant assignment of 'x' to itself.\n", errout.str());
 
         // #4073 (segmentation fault)
         check("void Foo::myFunc( int a )\n"
@@ -5514,7 +5514,7 @@ class TestOther : public TestFixture {
               "    BAR *x = getx();\n"
               "    x = x;\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'x' to itself.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:3]: (style) Redundant assignment of 'x' to itself.\n", errout.str());
 
         // #2502 - non-primitive type -> there might be some side effects
         check("void foo()\n"
@@ -5546,7 +5546,7 @@ class TestOther : public TestFixture {
               "void f() {\n"
               "    i = i;\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'i' to itself.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:3]: (style) Redundant assignment of 'i' to itself.\n", errout.str());
 
         // #4291 - id for variables accessed through 'this'
         check("class Foo {\n"
@@ -5556,7 +5556,7 @@ class TestOther : public TestFixture {
               "void Foo::func() {\n"
               "    this->var = var;\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:6]: (warning) Redundant assignment of 'this->var' to itself.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:6]: (style) Redundant assignment of 'this->var' to itself.\n", errout.str());
 
         check("class Foo {\n"
               "    int var;\n"
@@ -5575,7 +5575,7 @@ class TestOther : public TestFixture {
               "void f() {\n"
               "    struct callbacks ops = { .s = ops.s };\n"
               "}");
-        TODO_ASSERT_EQUALS("[test.cpp:6]: (warning) Redundant assignment of 'something' to itself.\n", "", errout.str());
+        TODO_ASSERT_EQUALS("[test.cpp:6]: (style) Redundant assignment of 'something' to itself.\n", "", errout.str());
 
         check("class V\n"
               "{\n"
@@ -5590,9 +5590,9 @@ class TestOther : public TestFixture {
               "    }\n"
               "    double x, y, z;\n"
               "};");
-        ASSERT_EQUALS("[test.cpp:10]: (warning) Redundant assignment of 'x' to itself.\n"
-                      "[test.cpp:10]: (warning) Redundant assignment of 'y' to itself.\n"
-                      "[test.cpp:10]: (warning) Redundant assignment of 'z' to itself.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:10]: (style) Redundant assignment of 'x' to itself.\n"
+                      "[test.cpp:10]: (style) Redundant assignment of 'y' to itself.\n"
+                      "[test.cpp:10]: (style) Redundant assignment of 'z' to itself.\n", errout.str());
 
         check("void f(int i) { i = !!i; }");
         ASSERT_EQUALS("", errout.str());
@@ -5602,7 +5602,7 @@ class TestOther : public TestFixture {
               "    int &ref = x;\n"
               "    ref = x;\n"
               "}\n");
-        ASSERT_EQUALS("[test.cpp:4]: (warning) Redundant assignment of 'ref' to itself.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:4]: (style) Redundant assignment of 'ref' to itself.\n", errout.str());
 
         check("class Foo {\n" // #9850
               "    int i{};\n"
@@ -7057,7 +7057,7 @@ class TestOther : public TestFixture {
               "        int var = buffer[index - 1];\n"
               "        return buffer[index - 1] - var;\n"  // <<
               "}");
-        ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) Same expression on both sides of '-'.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) Same expression on both sides of '-' because 'buffer[index-1]' and 'var' represent the same value.\n", errout.str());
     }
 
     void duplicateExpression13() { //#7899
@@ -10750,8 +10750,8 @@ class TestOther : public TestFixture {
               "  int x = x = y + 1;\n"
               "}", "test.c");
         ASSERT_EQUALS(
-            "[test.c:2]: (warning) Redundant assignment of 'x' to itself.\n"
-            "[test.c:2]: (warning) Redundant assignment of 'x' to itself.\n",   // duplicate
+            "[test.c:2]: (style) Redundant assignment of 'x' to itself.\n"
+            "[test.c:2]: (style) Redundant assignment of 'x' to itself.\n",   // duplicate
             errout.str());
     }