Skip to content

Added regression test for ticket #13474 #7536

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

orbitcowboy
Copy link
Collaborator

No description provided.

@danmar
Copy link
Owner

danmar commented May 17, 2025

could you lookup if there was such tests added already when this FN was fixed. which SHA fixed the FNs?
(It is supposed to be fixed during last 5 months)

@danmar
Copy link
Owner

danmar commented May 17, 2025

The test is unfortunate because it does not detect a regression. There are two possible ValueFlow solutions for this test:

  • either ValueFlow can determine that both i and j will be 0 and then when those are are multiplied there will be division by zero. Both loops are required for this.
  • or it can determine that whenever j is 0 then i*j is 0 and then there is division by zero. only the inner loop is required for this.

if valueflow handle both and then there is a regression so valueflow only handle one of them then this test does not detect that regression.

@danmar
Copy link
Owner

danmar commented May 17, 2025

either ValueFlow can determine that both i and j will be 0 and then when those are are multiplied there will be division by zero. Both loops are required for this.

A test for that could be:

    for (int i=2; i >= 0; i--) {
        for (int j=2; j >= 0; j--) {
            result = 20 / (i + j);
        }
    }

now it can only say that i + j is zero if both operand values are known.

or it can determine that whenever j is 0 then i*j is 0 and then there is division by zero. only the inner loop is required for this.

A proper test for that would be to use only the inner loop and let i value be unknown.

void foo(int i) {
    for (int j=2; j >= 0; j--)
        result = 20  / (i * j);
}

check("void f(void) {\n"
" for (int i = 2; i >= 0; --i) {\n"
" for (int j = 1; j >= 0; --j) {\n"
" int result = 42 / (i * j);\n" // << Division by zero when i or j is 0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still dislike this test as it won't detect if there is a regression.

// single for loop
check("void f(void) {\n"
" for (int j = 1; j >= 0; --j) {\n"
" int result = 42 / j;\n" // << Division by zero when j is 0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is same as zeroDiv19

@orbitcowboy
Copy link
Collaborator Author

@danmar Thanks for your comments. Test cases are now updated in accordance. It's good that you have provided these additional case, where (i +j) are summed up. It turned out that this case is a false negative (ref. to https://trac.cppcheck.net/ticket/13874).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants