From a5f4c5d0ebbed8d7802e4b11c27734c304b3e722 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sat, 25 Aug 2018 07:25:31 -0500 Subject: [PATCH] Improve message for same expressions (#1349) * Improve message for same expressions * Update message --- lib/checkother.cpp | 15 +++++++++++---- test/testother.cpp | 32 ++++++++++++++++---------------- 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 22e33a273..4e5e03e4f 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2057,12 +2057,19 @@ void CheckOther::duplicateExpressionError(const Token *tok1, const Token *tok2, const std::string& expr1 = tok1 ? tok1->expressionString() : "x"; const std::string& expr2 = tok2 ? tok2->expressionString() : "x"; - std::string endingPhrase = expr1 == expr2 ? "." : - " because the value of '" + expr1 + "' and '" + expr2 + "' are the same."; - const std::string& op = opTok ? opTok->str() : "&&"; + std::string msg = "Same expression on both sides of \'" + op + "\'"; + if (expr1 != expr2) { + std::string exprMsg = "The expression \'" + expr1 + " " + op + " " + expr2 + "\' is always "; + if(Token::Match(opTok, "==|>=|<=")) + msg = exprMsg + "true"; + else if(Token::Match(opTok, "!=|>|<")) + msg = exprMsg + "false"; + msg += " because '" + expr1 + "' and '" + expr2 + "' represent the same value"; + } - reportError(errors, Severity::style, "duplicateExpression", "Same expression on both sides of \'" + op + "\'" + endingPhrase + "\n" + + reportError(errors, Severity::style, "duplicateExpression", msg + ".\n" "Finding the same expression on both sides of an operator is suspicious and might " "indicate a cut and paste or logic error. Please examine this code carefully to " "determine if it is correct.", CWE398, false); diff --git a/test/testother.cpp b/test/testother.cpp index 3e55fc53f..c51f08fdf 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -3533,12 +3533,12 @@ private: check("void foo(int a, int b) {\n" " if ((a < b) && (b > a)) { }\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Same expression on both sides of '&&' because the value of 'aa' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Same expression on both sides of '&&' because 'aa' represent the same value.\n", errout.str()); check("void foo(int a, int b) {\n" " if ((a <= b) && (b >= a)) { }\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Same expression on both sides of '&&' because the value of 'a<=b' and 'b>=a' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Same expression on both sides of '&&' because 'a<=b' and 'b>=a' represent the same value.\n", errout.str()); check("void foo() {\n" " if (x!=2 || y!=3 || x!=2) {}\n" @@ -3662,7 +3662,7 @@ private: check("void foo(int a, int b) {\n" " if ((b + a) | (a + b)) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Same expression on both sides of '|' because the value of 'b+a' and 'a+b' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Same expression on both sides of '|' because 'b+a' and 'a+b' represent the same value.\n", errout.str()); check("void foo(const std::string& a, const std::string& b) {\n" " return a.find(b+\"&\") || a.find(\"&\"+b);\n" @@ -3677,7 +3677,7 @@ private: check("void foo(double a, double b) {\n" " if ((b + a) > (a + b)) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Same expression on both sides of '>' because the value of 'b+a' and 'a+b' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) The expression 'b+a > a+b' is always false because 'b+a' and 'a+b' represent the same value.\n", errout.str()); check("void f(int x) {\n" " if ((x == 1) && (x == 0x00000001))\n" @@ -3916,13 +3916,13 @@ private: " const int i = sizeof(int);\n" " if ( i != sizeof (int)){}\n" "}\n"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Same expression on both sides of '!=' because the value of 'i' and 'sizeof(int)' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The expression 'i != sizeof(int)' is always false because 'i' and 'sizeof(int)' represent the same value.\n", errout.str()); check("void f() {\n" " const int i = sizeof(int);\n" " if ( sizeof (int) != i){}\n" "}\n"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Same expression on both sides of '!=' because the value of 'sizeof(int)' and 'i' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The expression 'sizeof(int) != i' is always false because 'sizeof(int)' and 'i' represent the same value.\n", errout.str()); check("void f(int a = 1) { if ( a != 1){}}\n"); ASSERT_EQUALS("", errout.str()); @@ -3931,21 +3931,21 @@ private: " int a = 1;\n" " if ( a != 1){} \n" "}\n"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Same expression on both sides of '!=' because the value of 'a' and '1' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The expression 'a != 1' is always false because 'a' and '1' represent the same value.\n", errout.str()); check("void f() {\n" " int a = 1;\n" " int b = 1;\n" " if ( a != b){} \n" "}\n"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3] -> [test.cpp:4]: (style) Same expression on both sides of '!=' because the value of 'a' and 'b' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3] -> [test.cpp:4]: (style) The expression 'a != b' is always false because 'a' and 'b' represent the same value.\n", errout.str()); check("void f() {\n" " int a = 1;\n" " int b = a;\n" " if ( a != b){} \n" "}\n"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) Same expression on both sides of '!=' because the value of 'a' and 'b' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) The expression 'a != b' is always false because 'a' and 'b' represent the same value.\n", errout.str()); check("void use(int);\n" "void f() {\n" @@ -3954,7 +3954,7 @@ private: " use(b);\n" " if ( a != 1){} \n" "}\n"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:6]: (style) Same expression on both sides of '!=' because the value of 'a' and '1' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:6]: (style) The expression 'a != 1' is always false because 'a' and '1' represent the same value.\n", errout.str()); check("void use(int);\n" "void f() {\n" @@ -3978,7 +3978,7 @@ private: " void f() {\n" " if ( a != 1){} \n" "}\n"); - ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:3]: (style) Same expression on both sides of '!=' because the value of 'a' and '1' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:3]: (style) The expression 'a != 1' is always false because 'a' and '1' represent the same value.\n", errout.str()); check("int a = 1;\n" " void f() {\n" @@ -3990,7 +3990,7 @@ private: " static const int a = 1;\n" " if ( a != 1){} \n" "}\n"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Same expression on both sides of '!=' because the value of 'a' and '1' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The expression 'a != 1' is always false because 'a' and '1' represent the same value.\n", errout.str()); check("void f() {\n" " static int a = 1;\n" @@ -4003,7 +4003,7 @@ private: " if ( a != 1){\n" " a++;\n" " }}\n"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Same expression on both sides of '!=' because the value of 'a' and '1' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The expression 'a != 1' is always false because 'a' and '1' represent the same value.\n", errout.str()); check("void f(int b) {\n" " int a = 1;\n" @@ -4082,7 +4082,7 @@ private: " int a = 1;\n" " while ( a != 1){}\n" "}\n"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Same expression on both sides of '!=' because the value of 'a' and '1' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The expression 'a != 1' is always false because 'a' and '1' represent the same value.\n", errout.str()); check("void f() { int a = 1; while ( a != 1){ a++; }}\n"); ASSERT_EQUALS("", errout.str()); @@ -4098,7 +4098,7 @@ private: " if( i != 0 ) {}\n" " }\n" "}\n"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Same expression on both sides of '!=' because the value of 'i' and '0' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The expression 'i != 0' is always false because 'i' and '0' represent the same value.\n", errout.str()); check("void f() {\n" " for(int i = 0; i < 10;) {\n" @@ -4139,7 +4139,7 @@ private: " b++;\n" " }\n" "}\n"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) Same expression on both sides of '!=' because the value of 'a' and '1' are the same.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) The expression 'a != 1' is always false because 'a' and '1' represent the same value.\n", errout.str()); } void duplicateExpressionTernary() { // #6391