Repository navigation
Fix includes returning true for a run of equal elements not in the first range - #7871
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
 |
| Layer / File(s) | Summary |
|---|---|
Restrict early success to the final chunk libs/core/algorithms/include/hpx/parallel/algorithms/includes.hpp |
The algorithm returns early for an empty remainder only when it ends at last2. |
Test chunk-boundary behavior libs/core/algorithms/tests/regressions/includes_chunk_boundary.cpp, libs/core/algorithms/tests/regressions/CMakeLists.txt |
The new regression test checks specific boundary cases and compares generated sorted multisets with std::includes across static chunk sizes. The regression test is added to the existing test list. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related PRs
- TheHPXProject/hpx#7860: This PR changes chunk iterators in the parallel partitioner, while this change handles
hpx::includesbehavior at partition boundaries. - TheHPXProject/hpx#7844: Both PRs address duplicate-run handling across partitions in parallel set algorithms.
Merge Risk: 
b143f
-
No actionable merge-blocking risk is identified; the change is ready for normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title names the specific defect fixed: parallel includes returning true for a run of equal elements outside the first range. The changeset changes includes.hpp so that only the last chunk returns … Description check The description states the bug, the chunk-boundary mechanism, the proposed early-return change, and the new includes_chunk_boundary regression test. It also reports test results. This matches the chan… Linked Issues check Issue #7870requires parallelhpx::includesto returnfalsewhen the second sorted range contains a value absent from the first. The implementation now returns early only whenpart_end == last2.…Out of Scope Changes check The changed files implement issue #7870, register its regression test, and add focused coverage for the chunk-boundary defect. No unrelated change is established by the supplied pull request evidence.Full details: Title check
-
Explanation
The title names the specific defect fixed: parallel includes returning true for a run of equal elements outside the first range. The changeset changes includes.hpp so that only the last chunk returns early, and it adds a regression test for this case. The title matches the main change.
Full details: Description check
-
Explanation
The description states the bug, the chunk-boundary mechanism, the proposed early-return change, and the new includes_chunk_boundary regression test. It also reports test results. This matches the changeset, so the check passes.
Full details: Linked Issues check
-
Explanation
Issue
#7870requires parallelhpx::includesto returnfalsewhen the second sorted range contains a value absent from the first. The implementation now returns early only whenpart_end == last2. Otherwise, it verifies the run that starts in the next chunk. The regression test covers static chunk sizes 2 and 3 and compares static chunk sizes 1 through 4 withstd::includesover sorted multisets.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
|
Can one of the admins verify this patch? |
Up to standards ✅🟢 Issues
|
|
Thanks! HPX v2.0.0 has been released and master now contains it. Could you please rebase this PR onto the current master so that CI runs against the released code base? We will merge it once that is done. |
…rst range includes partitions the second range into chunks. A chunk skips its leading run of elements equal to its first element, relying on the previous chunk to have verified that run by extending its own range over it. A chunk that consisted only of such elements returned true right away and therefore never extended over the run that starts at the next chunk, which in turn skipped it. That run was never verified, so includes could return true although the second range contained elements that are not in the first range. Only return early if the chunk is the last one, otherwise continue and verify the run starting at the next chunk. Add a regression test with the minimal failing cases and an exhaustive comparison with std::includes for small sorted multisets. Fixes TheHPXProject#7870 Signed-off-by: Vaatsalya Srivastava <[email protected]>
b143fbc to
9773d0c
Compare
|
Rebased onto the current master (974ae9d). Thanks! |
hkaiser
left a comment
hkaiser
left a comment
There was a problem hiding this comment.
Thanks for the rebase, this looks right to me. A chunk that consists only of the run already covered by the previous chunk now verifies the run starting at the next chunk instead of returning early, which is exactly what the next chunk relies on. The exhaustive comparison against std::includes covers the boundary cases nicely. The macOS failure in static_chunker_2282 is unrelated.
Fixes #7870
Proposed Changes
includes, a chunk of the second range that consists only of elementscovered by the previous chunk no longer returns early (unless it is the
last chunk). It continues and verifies the run of equal elements that
starts at the next chunk. Previously this run was skipped by the next
chunk, which relies on the previous one to have verified it, so nobody
checked it and
includescould returntruealthough the second rangecontained elements that are not in the first range.
includes_chunk_boundary: the minimal failingcases for static chunk sizes 2 and 3, and an exhaustive comparison with
std::includesfor all sorted multisets over{0, 1, 2}with up to 6 and8 elements for static chunk sizes 1 to 4.
Any background context you want to provide?
Without this change the new test fails (151 failed checks); with it, it passes.
The existing
includesandincludes_empty_rangestests pass as well.Checklist
Not all points below apply to all pull requests.