From c57eb4d83418b7c7e2deb10e0450590b4b1c37fd Mon Sep 17 00:00:00 2001 From: Paul Date: Fri, 4 Apr 2025 16:01:18 -0500 Subject: [PATCH 1/4] Fix 13750: false negative: knownConditionTrueFalse/identicalConditionAfterEarlyExit --- lib/forwardanalyzer.cpp | 61 ++++++++++++++++++++++++++++++----------- test/testvalueflow.cpp | 8 ++++++ 2 files changed, 53 insertions(+), 16 deletions(-) diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 339c814280d..976d064ff1c 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -202,7 +202,8 @@ namespace { template )> Progress traverseConditional(T* tok, F f, bool traverseUnknown) { - if (Token::Match(tok, "?|&&|%oror%") && tok->astOperand1() && tok->astOperand2()) { + Analyzer::Action action = analyzer->analyze(tok, Analyzer::Direction::Forward); + if (action.isNone() && Token::Match(tok, "?|&&|%oror%") && tok->astOperand1() && tok->astOperand2()) { const T* condTok = tok->astOperand1(); T* childTok = tok->astOperand2(); bool checkThen, checkElse; @@ -227,12 +228,18 @@ namespace { if (traverseRecursive(childTok, f, traverseUnknown) == Progress::Break) return Break(); } + } else { + return f(tok, action); } return Progress::Continue; } Progress update(Token* tok) { Analyzer::Action action = analyzer->analyze(tok, Analyzer::Direction::Forward); + return update(tok, action); + } + + Progress update(Token* tok, Analyzer::Action action) { actions |= action; if (!action.isNone() && !analyzeOnly) analyzer->update(tok, action, Analyzer::Direction::Forward); @@ -245,30 +252,52 @@ namespace { return Break(Analyzer::Terminate::Modified); return Progress::Continue; } + + struct AsUpdate { + ForwardTraversal* self = nullptr; + + AsUpdate(ForwardTraversal* self) : self(self) {} + + template + Progress operator()(Ts... xs) const { + assert(self); + return self->update(xs...); + } + }; Progress updateTok(Token* tok, Token** out = nullptr) { - auto f = [this](Token* tok2) { - return update(tok2); - }; - return traverseTok(tok, f, false, out); + return traverseTok(tok, AsUpdate{this}, false, out); } Progress updateRecursive(Token* tok) { - auto f = [this](Token* tok2) { - return update(tok2); - }; - return traverseRecursive(tok, f, false); + return traverseRecursive(tok, AsUpdate{this}, false); } + struct AsAnalyze { + ForwardTraversal* self = nullptr; + Analyzer::Action * result = nullptr; + + AsAnalyze(ForwardTraversal* self, Analyzer::Action* result) : self(self), result(result) {} + + Progress operator()(const Token* tok) const { + assert(self); + assert(result); + return (*this)(tok, self->analyzer->analyze(tok, Analyzer::Direction::Forward)); + } + + Progress operator()(const Token*, Analyzer::Action action) const { + assert(self); + assert(result); + *result = action; + if (result->isModified() || result->isInconclusive()) + return self->Break(); + return Progress::Continue; + } + }; + Analyzer::Action analyzeRecursive(const Token* start) { Analyzer::Action result = Analyzer::Action::None; - auto f = [&](const Token* tok) { - result = analyzer->analyze(tok, Analyzer::Direction::Forward); - if (result.isModified() || result.isInconclusive()) - return Break(); - return Progress::Continue; - }; - traverseRecursive(start, f, true); + traverseRecursive(start, AsAnalyze{this, &result}, true); return result; } diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index ed942906efc..39c3cc5cf6d 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -6364,6 +6364,14 @@ class TestValueFlow : public TestFixture { " return false;\n" "}\n"; ASSERT_EQUALS(true, testValueOfXKnown(code, 6U, 0)); + + code = "bool f(bool b1, bool b2) {\n" + " if (b1 && b2)\n" + " return;\n" + " int x = b1 && b2;\n" + " return x;\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXKnown(code, 5U, 0)); } static std::string isPossibleContainerSizeValue(std::list values, From d6bdc1356c7d69cd261ae9741a1b309cd01df5b1 Mon Sep 17 00:00:00 2001 From: Paul Date: Fri, 4 Apr 2025 16:02:04 -0500 Subject: [PATCH 2/4] Format --- lib/forwardanalyzer.cpp | 18 +++++++++++------- test/testvalueflow.cpp | 10 +++++----- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 976d064ff1c..7876ad35f61 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -239,7 +239,8 @@ namespace { return update(tok, action); } - Progress update(Token* tok, Analyzer::Action action) { + Progress update(Token* tok, Analyzer::Action action) + { actions |= action; if (!action.isNone() && !analyzeOnly) analyzer->update(tok, action, Analyzer::Direction::Forward); @@ -252,16 +253,17 @@ namespace { return Break(Analyzer::Terminate::Modified); return Progress::Continue; } - + struct AsUpdate { ForwardTraversal* self = nullptr; AsUpdate(ForwardTraversal* self) : self(self) {} template - Progress operator()(Ts... xs) const { + Progress operator()(Ts... xs) const + { assert(self); - return self->update(xs...); + return self->update(xs ...); } }; @@ -275,17 +277,19 @@ namespace { struct AsAnalyze { ForwardTraversal* self = nullptr; - Analyzer::Action * result = nullptr; + Analyzer::Action* result = nullptr; AsAnalyze(ForwardTraversal* self, Analyzer::Action* result) : self(self), result(result) {} - Progress operator()(const Token* tok) const { + Progress operator()(const Token* tok) const + { assert(self); assert(result); return (*this)(tok, self->analyzer->analyze(tok, Analyzer::Direction::Forward)); } - Progress operator()(const Token*, Analyzer::Action action) const { + Progress operator()(const Token*, Analyzer::Action action) const + { assert(self); assert(result); *result = action; diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 39c3cc5cf6d..22f9bfd20b4 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -6366,11 +6366,11 @@ class TestValueFlow : public TestFixture { ASSERT_EQUALS(true, testValueOfXKnown(code, 6U, 0)); code = "bool f(bool b1, bool b2) {\n" - " if (b1 && b2)\n" - " return;\n" - " int x = b1 && b2;\n" - " return x;\n" - "}\n"; + " if (b1 && b2)\n" + " return;\n" + " int x = b1 && b2;\n" + " return x;\n" + "}\n"; ASSERT_EQUALS(true, testValueOfXKnown(code, 5U, 0)); } From 6a2a196585181e8f30d4985e49b65fc08870aaf7 Mon Sep 17 00:00:00 2001 From: Paul Date: Sat, 5 Apr 2025 13:12:05 -0500 Subject: [PATCH 3/4] Make constructor explicit --- lib/forwardanalyzer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 7876ad35f61..92d97d03d2b 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -257,7 +257,7 @@ namespace { struct AsUpdate { ForwardTraversal* self = nullptr; - AsUpdate(ForwardTraversal* self) : self(self) {} + explicit AsUpdate(ForwardTraversal* self) : self(self) {} template Progress operator()(Ts... xs) const From 205be498734ffbbe55d45ea6857e9e95d5873123 Mon Sep 17 00:00:00 2001 From: Paul Date: Sat, 12 Apr 2025 10:58:45 -0500 Subject: [PATCH 4/4] Fix tidy --- lib/forwardanalyzer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 92d97d03d2b..9da1486bcdb 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -288,7 +288,7 @@ namespace { return (*this)(tok, self->analyzer->analyze(tok, Analyzer::Direction::Forward)); } - Progress operator()(const Token*, Analyzer::Action action) const + Progress operator()(const Token* /*unused*/, Analyzer::Action action) const { assert(self); assert(result);