8000 Added regression test for ticket #13474 by orbitcowboy · Pull Request #7536 · danmar/cppcheck · GitHub
[go: up one dir, main page]
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.

Sign up for GitHub

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

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

Added regression test for ticket #13474 #7536

wants to merge 4 commits into from

Conversation

orbitcowboy
Copy link
Collaborator

No description provided.

danmar
danmar reviewed May 17, 2025
View reviewed changes
@danmar
Copy link
Owner
danmar commented May 17, 2025 •
edited
Loading

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 •
edited
Loading

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);
}

danmar
danmar reviewed May 17, 2025
View reviewed changes
test/testother.cpp Outdated
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.

test/testother.cpp Outdated
// 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).

danmar
danmar reviewed May 20, 2025
View reviewed changes
Copy link
Owner
@danmar danmar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't feel very good about these.

I feel these are not unit tests. These tests are not related to the checker implementation at all. There is no special handling for "for loops" in the checker. So adding various such tests does not improve the testing nor code coverage of the checker code. And it's the checker code that should be tested here.

I have a suggestion. Do you want to add some kind of higher level test suite instead? In some separate folder. Feel free to do it in the examples folder. Imho it could also be a separate "test suite" that does not have any fixes.. I don't have strong opinions.

The ticket only talks about division by zero but I can easily envision various invalid memory/arithmetic/etc operations in loops.. we should look at all checkers at once so we don't have to rediscover this pattern over and over..

@danmar
Copy link
Owner
danmar commented May 20, 2025 •
edited
Loading

Off the top of my head I believe a test suite could also for instance have similar loops with:

  • modulo with zero
  • array index underrun
  • shift overflow
  • signed integer underflow
  • invalid function argument (when 0 is not in the valid range)
    ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danmar danmar danmar left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3FA5 No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@orbitcowboy @danmar
0