fix #5591: Add check for meaningless comma operator in if statement, misplaced ')'

[clock999]

https://trac.cppcheck.net/ticket/5591

Description:
int f2(int, int = 0) {....}

void f()
{
if(f2(0), 0) //actually mean f2(0, 0)
{
....
}
}

[chrchr-github]

Thanks for your contribution. Note that these issues seem to be diagnosed already by current compilers.

[clock999]

Hi CHR, many thanks for your comments, and I have updated the commit. But there is a failed case tested on the github for the commit, that seems not the problem of the committed code. Seems the network traffic‌ caused a overtime. But I am not sure. Do you know how to trigger the checks to run again? Thanks a lot!

[chrchr-github]

The macos runners can be very slow sometimes. I have restarted the failing run.

[danmar]

the PR description says that you want to detect:

if(f2(0), 0) //actually mean f2(0, 0)

well yes that looks like a possible bug. there seems to be misplaced parenthesis

but I fear your checker is much more verbose. you warn whenever there is a comma in a if/while. no matter if there are function calls.

If the condition says:

    if (x,y)

I agree the code looks weird.. but it's not a likely misplaced parenthesis and I am not convinced that the comma is a bug. It looks intentional.

[danmar]

I also want to point out that misra has a rule that forbids comma operators. But I still feel sympathy for this checker if we make it more specific and it can detect actual bugs and not just warn about random comma operators.

[clock999]

I also want to point out that misra has a rule that forbids comma operators. But I still feel sympathy for this checker if we make it more specific and it can detect actual bugs and not just warn about random comma operators.

Hi Danmer, according to the description of the bug 5591, seems the user just want the cppcheck to help to give some warning to avoid unintended spelling mistakes which are grammatically correct. But it is a logical error during running. So maybe it is a little difficult to completely decide if the code is intended or not by a static analysis. Maybe we can find out the most likely possibility and provide a warning. The case described on the bug usually happened when using a function with default arguments, as the default arguments might be ignored and misplaced outside the parentheses. So is it ok that we just focus on the functions with default args called in an if or while condition statement?

[danmar]

So maybe it is a little difficult to completely decide if the code is intended or not by a static analysis.

yeah.. and when you say "a little difficult" you probably mean "impossible" right ? :-)

our approach is to make the checker "passive" then and warn only about the most obvious bugs. and I think your suggestion resonate well with that.

The case described on the bug usually happened when using a function with default arguments, as the default arguments might be ignored and misplaced outside the parentheses. So is it ok that we just focus on the functions with default args called in an if or while condition statement?

Yes that sounds much better to me. If such function appear immediately before the comma operator then I would think it seems fair to write a warning.

[danmar]

• please add some tests also in the testother.cpp
• update CheckOther::getErrorMessages
• update classInfo in checkother.h

[danmar]

you can write Token::Match(parent->previous(), "if|while (") instead

[danmar]

I like overall to be over-explicit. but this && tok->previous()->link()->str() == "(" is too redundant please remove that.

[danmar]

explain why there must be a known value?

[clock999]

Hi Danmar, sorry, that code is thoughtless. I will update the commit. I make a sample to explain my thinking.

int func(int a, int b = 100) {
    return 1;
}

int main() {
    int x = 200;
    int y = 100;
    if (func(100), x) {} //case 1, report warning
    if (func(100), true) {} //case 2, report warning
    if (func(100), 100) {} //case 3, report warning
    
    if (func(100), func(100)) {} //case 4, not report warning
    if (func(100), x + y) {} //case 5, not report warning
    return 100;
}

For case 4 and case 5, seems the programmer is more likely intended to do that. But I am not quite sure about that.

[danmar]

@clock999 hmm personally I don't see why case 4 and case 5 looks more intentional than cases 1-3. I would warn about those too.

[danmar]

you don't have to do it in this PR but I think this warning could be more generic and written for other statements also. Example:

     x = 2 * (3 + foo(4,5),6) + 7;

It should check that the comma is inside parentheses.

[chrchr-github]

lib/checkother.cpp:4384:50: style: Call to 'Token::previous()' followed by 'Token::link()' can be simplified. [redundantNextPrevious]
                    const Function * func = tok->previous()->link()->previous()->function();

Use tok->linkAt(-1)->previous()->function() instead.

[clock999]

lib/checkother.cpp:4384:50: style: Call to 'Token::previous()' followed by 'Token::link()' can be simplified. [redundantNextPrevious]
                    const Function * func = tok->previous()->link()->previous()->function();

Use tok->linkAt(-1)->previous()->function() instead.

Hi CHR, thanks a lot for your help! With the latest submit, the github report test failures. It is caused by running the testrunner. But all the tests can be passed when I run the testrunner locally on my pc.
When running the test case 'suspiciousComma' in the testother.cpp, on github, the actual output of the test case is different from the one on my local pc.
On github:

Expected: 
[test.cpp:3]: (style) There is an suspicious comma expression used as a condition in an if/while condition statement.\n

Actual: 
[test.cpp:3:15]: (style) There is an suspicious comma expression used as a condition in an if/while condition statement. [warnSuspiciousComma]\n

Local:

Actual: 
[test.cpp:3]: (style) There is an suspicious comma expression used as a condition in an if/while condition statement.\n

Do you know what's the reason? Thanks!

[chrchr-github]

Do you know what's the reason? Thanks!

Maybe you need to rebase onto main? I think there were some recent changes involving the column number.

[chrchr-github]

lib/checkother.cpp:4399:26: style: Call to 'Token::tokAt()' followed by 'Token::str()' can be simplified. [redundantNextPrevious]
                if (tok->tokAt(-1)->str() == ")") {
                         ^

should be tok->strAt(-1).

[danmar]

suspicious comma immediately after function call in if/while condition statement.

[danmar]

Suggested change

	reportError(tok, Severity::style, "suspiciousComma", "There is a suspicious comma expression in an if/while condition statement.");
	reportError(tok, Severity::style, "suspiciousComma", "There is a suspicious comma expression directly after a function call in an if/while condition, is there a misplaced paranthesis?");

[danmar]

You have added tests that is good but it feels too little with just 2 tests.

You could add a test that demonstrates that there must be a function call. i.e.:

    if (x,100) {}  // no warning

And a test where there can't be additional parameters:

    if (foo(100,200),100) {}  // no warning

[danmar]

I feel that it would be good to run test-my-pr.py script and investigate if the warnings look correct..

[clock999]

I feel that it would be good to run test-my-pr.py script and investigate if the warnings look correct..

Hi Danmar, while running the test-my-pr.py, there is a problem. The function compile_cppcheck in the file tools/donate_cpu_lib.py failed. The compiling error is that "undefined reference to `simplecpp::TokenList::TokenList". I checked the folder cppcheck/build, and found that there is no simplecpp.o there. Seems simplecpp.cpp is not compiled.

[danmar]

@clock999 thanks! Sometimes old builds mess it up because the script uses different build flags etc.. so if you remove the build folder and all old *.o files etc you might have then restart the test-my-pr.py script..

I would also like that you add a line about this new checker in the releasenotes.txt

I would like to see what happens if you warn also when there are not known values.

If the code is something like:

     if (f(1),2)

then basically I think we should report a "known condition value".. maybe the "known condition value" checker does not handle comma as it should..

[clock999]

@clock999 thanks! Sometimes old builds mess it up because the script uses different build flags etc.. so if you remove the build folder and all old *.o files etc you might have then restart the test-my-pr.py script..

I would also like that you add a line about this new checker in the releasenotes.txt

I would like to see what happens if you warn also when there are not known values.

If the code is something like:

     if (f(1),2)

then basically I think we should report a "known condition value".. maybe the "known condition value" checker does not handle comma as it should..

HI Danmar, yes, CheckCondition::alwaysTrueFalse does not handle the comma expression. With below code, no message is reported for known condition value.

int main() {
    int x = 200;
    int y = 100;
    if(x,x,y) {}
    return 0;
}

When checking if it is in a condition statement, we ignored the comma case.

            const Token* condition = nullptr;
            {
                // is this a condition..
                const Token *parent = tok->astParent();
                while (Token::Match(parent, "%oror%|&&"))
                    parent = parent->astParent();
                if (!parent)
                    continue;
                if (parent->str() == "?" && precedes(tok, parent))
                    condition = parent;
                else if (Token::Match(parent->previous(), "if|while ("))
                    condition = parent->previous();
                else if (Token::simpleMatch(parent, "return"))
                    condition = parent;
                else if (parent->str() == ";" && parent->astParent() && parent->astParent()->astParent() &&
                         Token::simpleMatch(parent->astParent()->astParent()->previous(), "for ("))
                    condition = parent->astParent()->astParent()->previous();
                else if (Token::Match(tok, "%comp%"))
                    condition = tok;
                else
                    continue;
            }

Maybe we need:

                while (Token::Match(parent, "%oror%|&&|,"))  //add the comma case
                    parent = parent->astParent();

But this problem still can not be fixed just like that simply. isUsedAsBool will return false for the var y, and I did not dig further.
For the 'known condition value' problem, shell we need to raise another issue to track and fix it?

For the meaningless comma, if there are unknown values just after the comma, we will report a warning with the current submitted commit code.
For example:

void f(int a, int b = 0);
void foo(int var) {
    while (f(100), var) {}
}

I updated the test case for the unit tests.

[danmar]

HI Danmar, yes, CheckCondition::alwaysTrueFalse does not handle the comma expression. With below code, no message is reported for known condition value.

ok thanks for your investigation..

Let's just leave that CheckCondition::alwaysTrueFalse as it is for now.

[danmar]

I suggest that no warning is written here:

void foo(int x, int y=0) ;

void bar() {
    if (foo(1,2),1) {}
}

[danmar]

I have executed test-my-pr.py

I got some warnings in Poppler project for such code:

        if (obj = parser->getObj(true), !obj.isInt()) {

However this looks intentional as the getObj declaration is:

    Object getObj(bool simpleOnly = false, const unsigned char *fileKey = nullptr, CryptAlgorithm encAlgorithm = cryptRC4, int keyLength = 0, int objNum = 0, int objGen = 0, int recursion = 0, bool strict = false,
                  bool decryptString = true);

conversion from bool to pointer is likely not the intention. could you please fix these?

[danmar]

I would not be against some exception for a=f(), some expression using a. But I don't have a strong opinion you can decide about that.

[clock999]

I have executed test-my-pr.py

I got some warnings in Poppler project for such code:

        if (obj = parser->getObj(true), !obj.isInt()) {

However this looks intentional as the getObj declaration is:

    Object getObj(bool simpleOnly = false, const unsigned char *fileKey = nullptr, CryptAlgorithm encAlgorithm = cryptRC4, int keyLength = 0, int objNum = 0, int objGen = 0, int recursion = 0, bool strict = false,
                  bool decryptString = true);

conversion from bool to pointer is likely not the intention. could you please fix these?

HI Danmar, ok, I will check it. When running the test-my-pr.py, I find that the packages checked this time will be totally different from last time. And the download link of each package to be checked is returned from the server each time. So if I'd like to check the package Poppler seperately, is there any tool or script for downloading it?
I also executed the test-my-pr.py, but sometimes it will fail for network problems, or executing timeout, etc. Once failed, it will start from the beginning when being executed again. And all the packages are totally different from last time. It is quite a long time for the whole running.

[danmar]

I also executed the test-my-pr.py, but sometimes it will fail for network problems, or executing timeout, etc. Once failed, it will start from the beginning when being executed again. And all the packages are totally different from last time. It is quite a long time for the whole running.

ok ..

• I believe you can use --packages to specify the packages.. specify poppler path/url and it only checks that
• personally I have downloaded the packages and don't use network downloads.. you can try this script: cppcheck/tools/daca2-download.py

[clock999]

Hi Danmar, I checked the package Poppler and some of the downloaded packages with the updated commit, but I have not run the script test-my-pr.py for all the packages. I see there are more than 40000 packages to be downloaded with daca2-download.py.

[danmar]

I would like to add documentation about checkers.
I have this work in progress for "cstyleCast":
#7539
Could you add some such page for your new checker.
I think the key point to describe to users is what exact bugs did you intend to catch and can you write something about the philosophy. If a user gets a warning that he thinks is a false positive then with the document it should be possible for the user to determine if he does have the problem that you wanted to find, and if not.. would it make sense to add some further heuristics to avoid false positives.

[danmar]

I don't really understand && !tok->astOperand2()->hasKnownValue() .. is there any test that shows why that is needed?