Fix #11579 FN knownConditionTrueFalse with non-bool as bool parameter / #9450 string literal to bool conversion in function call #5338
Conversation
chrchr-github
changed the title
| @@ -1490,6 +1490,8 @@ void CheckCondition::alwaysTrueFalse() | |||
| else if (parent->str() == ";" && parent->astParent() && parent->astParent()->astParent() && | |||
| Token::simpleMatch(parent->astParent()->astParent()->previous(), "for (")) | |||
| condition = parent->astParent()->astParent()->previous(); | |||
| else if (isBooleanFuncArg(tok)) | |||
There was a problem hiding this comment.
This should probably go into the knownArgument checker.
There was a problem hiding this comment.
Looking at the test cases, knownArgument seems to be more about calculations with a known result, not simple conditions.
There was a problem hiding this comment.
It checks comparisons as well, but only for integral variables.
| @@ -488,7 +488,7 @@ struct ForwardTraversal { | |||
| if (allAnalysis.isIncremental()) | |||
| return Break(Analyzer::Terminate::Bail); | |||
| } else if (allAnalysis.isModified()) { | |||
| std::vector<ForwardTraversal> ftv = tryForkScope(endBlock, allAnalysis.isModified()); | |||
| std::vector<ForwardTraversal> ftv = tryForkScope(endBlock, /*isModified*/ true); | |||
There was a problem hiding this comment.
I think this is a FP. Yes its correct that it is always true, but it makes the code less clear about why it is being passed true.
There was a problem hiding this comment.
What if isModified() is expensive to compute?
There was a problem hiding this comment.
I would probably assign it to a variable and reuse it, but thats not the case here.
| if (!mImpl->mValues) | ||
| return false; | ||
| return std::any_of(mImpl->mValues->begin(), mImpl->mValues->end(), [](const ValueFlow::Value& value) { | ||
| return value.isIntValue() && (value.isKnown() || (value.intvalue == 0 && value.isImpossible())); |
There was a problem hiding this comment.
valueFlowInferCondition should make these values known, instead of having the checkers deal with this logic.
| @@ -912,7 +912,7 @@ static void compilePrecedence2(Token *&tok, AST_state& state) | |||
| } | |||
| compileBinOp(tok, state, compileScope); | |||
| } else if (tok->str() == "[") { | |||
| if (state.cpp && isPrefixUnary(tok, state.cpp) && Token::Match(tok->link(), "] (|{|<")) { // Lambda | |||
| if (state.cpp && isPrefixUnary(tok, /*cpp*/ true) && Token::Match(tok->link(), "] (|{|<")) { // Lambda | |||
There was a problem hiding this comment.
There is already a test case for this with knownArgument, we shouldn't be warning here:
check("struct A { int x; };"
"void g(int);\n"
"void f(int x) {\n"
" A y;\n"
" y.x = 1;\n"
" g(y.x);\n"
"}");
ASSERT_EQUALS("", errout.str());From https://github.com/danmar/cppcheck/blob/main/test/testother.cpp#L10987
There was a problem hiding this comment.
Not quite the same, since this PR is only about bool arguments. The code in the test case still seems weird.
Maybe we should only warn when there is an implicit conversion though.
There was a problem hiding this comment.
Maybe we should only warn when there is an implicit conversion though.
Yea the original issue seems to only be about pointer to bool conversion.
No description provided.