From 3f36d4b5f42a233d8e9c5884d1ffbe3373a18cad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Mon, 11 Dec 2017 22:10:00 +0100 Subject: [PATCH] try to clarify error message --- lib/checkstring.cpp | 15 +++++++++++---- lib/checkstring.h | 12 ++++++------ test/teststring.cpp | 8 ++++---- 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/lib/checkstring.cpp b/lib/checkstring.cpp index f6ab41d21..670d08a09 100644 --- a/lib/checkstring.cpp +++ b/lib/checkstring.cpp @@ -336,7 +336,7 @@ void CheckString::incorrectStringBooleanError(const Token *tok, const std::strin // always true: strcmp(str,"a")==0 || strcmp(str,"b") // TODO: Library configuration for string comparison functions //--------------------------------------------------------------------------- -void CheckString::overlappingStringComparisons() +void CheckString::deadStrcmp() { if (!_settings->isEnabled(Settings::WARNING)) return; @@ -399,14 +399,14 @@ void CheckString::overlappingStringComparisons() args2[1]->isLiteral() && args1[1]->str() != args2[1]->str() && isSameExpression(_tokenizer->isCPP(), true, args1[0], args2[0], _settings->library, true)) - overlappingStringComparisonsError(tok1, tok2); + deadStrcmpError(tok1, tok2); } } } } } -void CheckString::overlappingStringComparisonsError(const Token *eq0, const Token *ne0) +void CheckString::deadStrcmpError(const Token *eq0, const Token *ne0) { std::string eq0Expr(eq0 ? eq0->expressionString() : std::string("strcmp(x,\"abc\")")); if (eq0 && eq0->astParent()->str() == "!") @@ -414,7 +414,14 @@ void CheckString::overlappingStringComparisonsError(const Token *eq0, const Toke else eq0Expr += " == 0"; const std::string ne0Expr = (ne0 ? ne0->expressionString() : std::string("strcmp(x,\"def\")")) + " != 0"; - reportError(ne0, Severity::warning, "overlappingStringComparisons", "The comparison operator in '" + ne0Expr + "' should maybe be '==' instead, currently the expression '" + eq0Expr + "' is redundant."); + + const Token *ortok = eq0; + while (ortok && ortok->str() != "||") + ortok = ortok->astParent(); + + const std::string expression = ortok ? ortok->expressionString() : (eq0Expr + " || " + ne0Expr); + + reportError(ne0, Severity::warning, "deadStrcmp", "The expression '" + expression + "' has a dead string comparison. The expression is logically the same as \'" + ne0Expr + "\'."); } //--------------------------------------------------------------------------- diff --git a/lib/checkstring.h b/lib/checkstring.h index 430253687..8bdc405ed 100644 --- a/lib/checkstring.h +++ b/lib/checkstring.h @@ -57,7 +57,7 @@ public: checkString.strPlusChar(); checkString.checkSuspiciousStringCompare(); checkString.stringLiteralWrite(); - checkString.overlappingStringComparisons(); + checkString.deadStrcmp(); } /** @brief Run checks against the simplified token list */ @@ -85,8 +85,8 @@ public: /** @brief %Check for suspicious code that compares string literals for equality */ void checkAlwaysTrueOrFalseStringCompare(); - /** @brief %Check for overlapping string comparisons */ - void overlappingStringComparisons(); + /** @brief %Check for dead string comparison */ + void deadStrcmp(); /** @brief %Check for overlapping source and destination passed to sprintf() */ void sprintfOverlappingData(); @@ -101,7 +101,7 @@ private: void alwaysTrueStringVariableCompareError(const Token *tok, const std::string& str1, const std::string& str2); void suspiciousStringCompareError(const Token* tok, const std::string& var); void suspiciousStringCompareError_char(const Token* tok, const std::string& var); - void overlappingStringComparisonsError(const Token* tok1, const Token *tok2); + void deadStrcmpError(const Token* tok1, const Token *tok2); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckString c(nullptr, settings, errorLogger); @@ -115,7 +115,7 @@ private: c.incorrectStringBooleanError(nullptr, "\"Hello World\""); c.alwaysTrueFalseStringCompareError(nullptr, "str1", "str2"); c.alwaysTrueStringVariableCompareError(nullptr, "varname1", "varname2"); - c.overlappingStringComparisonsError(nullptr, nullptr); + c.deadStrcmpError(nullptr, nullptr); } static std::string myName() { @@ -130,7 +130,7 @@ private: "- suspicious condition (string literals as boolean)\n" "- suspicious comparison of a string literal with a char* variable\n" "- suspicious comparison of '\\0' with a char* variable\n" - "- overlapping string comparisons (strcmp(str,\"x\")==0 || strcmp(str,\"y\")!=0)\n"; + "- dead string comparison; (strcmp(str,\"x\")==0 || strcmp(str,\"y\")) is logically the same as (strcmp(str,\"y\") != 0)\n"; } }; /// @} diff --git a/test/teststring.cpp b/test/teststring.cpp index 05c7baf2f..41b0143c3 100644 --- a/test/teststring.cpp +++ b/test/teststring.cpp @@ -52,7 +52,7 @@ private: TEST_CASE(incorrectStringCompare); - TEST_CASE(overlappingStringComparisons); + TEST_CASE(deadStrcmp); } void check(const char code[], const char filename[] = "test.cpp") { @@ -581,11 +581,11 @@ private: ASSERT_EQUALS("", errout.str()); } - void overlappingStringComparisons() { + void deadStrcmp() { check("void f(const char *str) {\n" " if (strcmp(str, \"abc\") == 0 || strcmp(str, \"def\")) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) The comparison operator in 'strcmp(str,\"def\") != 0' should maybe be '==' instead, currently the expression 'strcmp(str,\"abc\") == 0' is redundant.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) The expression 'strcmp(str,\"abc\")==0||strcmp(str,\"def\")' has a dead string comparison. The expression is logically the same as 'strcmp(str,\"def\") != 0'.\n", errout.str()); check("struct X {\n" " char *str;\n" @@ -594,7 +594,7 @@ private: "void f(const struct X *x) {\n" " if (strcmp(x->str, \"abc\") == 0 || strcmp(x->str, \"def\")) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:6]: (warning) The comparison operator in 'strcmp(x->str,\"def\") != 0' should maybe be '==' instead, currently the expression 'strcmp(x->str,\"abc\") == 0' is redundant.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:6]: (warning) The expression 'strcmp(x->str,\"abc\")==0||strcmp(x->str,\"def\")' has a dead string comparison. The expression is logically the same as 'strcmp(x->str,\"def\") != 0'.\n", errout.str()); } };