Skip to content

tests: increase coverage for containerOutOfBounds (non-empty containers) #7733

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

Closed

Conversation

aryanrahar
Copy link

What
Add negative regression tests ensuring no containerOutOfBounds false positive when:

  • std::vector is initialized with {...} and indexed
  • std::array<int,2> is initializer-list initialized and indexed
  • std::vector has at least one element via push_back() and is indexed

Why
A previously reported FP existed around init-list vectors. These tests lock the fix and guard future regressions.

How
Added three free-function tests inside the existing STL test namespace and registered them in TestStl::run(). Pattern matches existing tests (check(...) + ASSERT_EQUALS("", errout_str())).

Results
ctest all green locally.

@chrchr-github
Copy link
Collaborator

Why A previously reported FP existed around init-list vectors. These tests lock the fix and guard future regressions.

Care to elaborate about that FP? In which version was it observed?

@aryanrahar
Copy link
Author

Why A previously reported FP existed around init-list vectors. These tests lock the fix and guard future regressions.

Care to elaborate about that FP? In which version was it observed?

Thanks for the review! To clarify: this PR is preventive coverage rather than fixing a currently failing case. I didn’t observe a false positive on current main; the goal is to lock in the expected “no diagnostic” behavior for non-empty containers created via initializer-list or after push_back().

Motivation came from reading the containerOutOfBounds logic and seeing several cfg tests/suppressions around bounds checks. These small negative tests document the intended behavior and guard against future regressions.

I can update the PR title/body to remove the “previously reported FP” wording if you prefer. Also happy to move/rename the tests to fit your conventions.

@firewave
Copy link
Collaborator

Thanks for your contribution.

The test names are extremely awkward (and could probably grouped into an existing function). No need for that comment either. And the flawed formatting will cause the build to fail. Also there is no need for include. And please use multi-line source in the input for better readability and dedicated lines for statements.

The name of the PR could also be adjusted to simply state "increased coverage".

While at it, it might make sense to check if there are also some positive tests which should be added.

@aryanrahar aryanrahar changed the title tests: prevent regression for containerOutOfBounds with initializer-list / non-empty containers tests: increase coverage for containerOutOfBounds (non-empty containers) Aug 13, 2025
@aryanrahar
Copy link
Author

Thanks for your contribution.

The test names are extremely awkward (and could probably grouped into an existing function). No need for that comment either. And the flawed formatting will cause the build to fail. Also there is no need for include. And please use multi-line source in the input for better readability and dedicated lines for statements.

The name of the PR could also be adjusted to simply state "increased coverage".

While at it, it might make sense to check if there are also some positive tests which should be added.

Thanks! I folded the checks into TestStl::outOfBoundsIndexExpression(), switched to multi-line sources (one statement per line), removed the includes/comment, and updated the title to “increase coverage”. ctest is green locally.

@firewave
Copy link
Collaborator

Thanks. The indentation is still wrong and please remove the empty lines.

Copy link

@aryanrahar
Copy link
Author

Thanks. The indentation is still wrong and please remove the empty lines.

Thanks!!!I have corrected the indentation, if there any empty lines present can you point the exact line!

@aryanrahar
Copy link
Author

Thanks. The indentation is still wrong and please remove the empty lines.

Thanks!!!I have corrected the indentation, if there any empty lines present can you point the exact line!

????

@chrchr-github
Copy link
Collaborator

The first test is an exact duplicate of an existing test (#12738). We already have tests similar to the other ones. The alleged previous FP could not be demonstrated.
I have a suspicion that this is AI-generated or otherwise done in bad faith.

@aryanrahar
Copy link
Author

The first test is an exact duplicate of an existing test (#12738). We already have tests similar to the other ones. The alleged previous FP could not be demonstrated. I have a suspicion that this is AI-generated or otherwise done in bad faith.

Thanks for the review and sorry for the noise. You’re right: the first case duplicates #12738 and the others are already covered. My “previous FP” wording was sloppy; I didn’t have a repro.

To address the concern - this was not AI-generated and there was no bad faith, I wrote and ran the changes myself, but I missed the existing coverage.

I’ll avoid overlapping tests going forward. If there are specific gaps you’d like help with, I’m happy to pick them up; otherwise I’ll grab a help-wanted / good-first-issue and come back with a focused PR.

@firewave
Copy link
Collaborator

I’ll avoid overlapping tests going forward. If there are specific gaps you’d like help with, I’m happy to pick them up; otherwise I’ll grab a help-wanted / good-first-issue and come back with a focused PR.

There is a couple of checks which are currently lacking tests:

https://trac.cppcheck.net/query?status=accepted&status=assigned&status=new&status=reopened&summary=~add+tests+for&col=id&col=summary&col=status&col=type&col=priority&col=milestone&col=component&order=priority

Please be aware that it is possible that these checks are disabled/broken/etc. so you might not be able to produce code which actually generates these warnings. But if that is the case it is still worth pointing out.

Regarding the formatting issues in this PR. We are using uncrustify to format our code. So no need to do manual adjustments.

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.

3 participants