Repository navigation
Fix parallel find_end comparing the wrong elements - #7767
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check The change addresses #7766. In the partitionedsequential_find_end_toverload,curradvances with the element passed byloop_idx_n, and the remaining pattern elements are read through*mid. Th…Out of Scope Changes check The changes stay within #7766. The production change fixes the identified partitionedfind_endcomparison. The added test covers the reported non-consecutive-value and non-incrementable-element case…Title check The title clearly and concisely describes the primary fix to parallel find_end behavior. Description check The description directly explains the bug, implementation, regression tests, affected policies, and validation results. Full details: Linked Issues check
-
Explanation
The change addresses
#7766. In the partitionedsequential_find_end_toverload,curradvances with the element passed byloop_idx_n, and the remaining pattern elements are read through*mid. The callback usesstd::iterator_traits<Iter1>::reference, so element types such asstd::stringare not incremented.test_find_end5compares HPX results withstd::find_endfor{2, 1}and string elements underseq,par, andpar_unseq, using random-access and forward iterators.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within
#7766. The production change fixes the identified partitionedfind_endcomparison. The added test covers the reported non-consecutive-value and non-incrementable-element cases. The direct<algorithm>include supports the newstd::find_endcomparison.
-
- Fix all pre-merge checks with AI
✨ 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.
Comment @coderabbitai help to get the list of available commands.
Up to standards ✅🟢 Issues
|
|
Can one of the admins verify this patch? |
util::loop_idx_n passes each element to the callback, not an iterator to
it, but the partitioned find_end treated it as an iterator: after the
first element of the subsequence matched, it incremented a copy of that
element instead of moving to the next one, and compared the rest of the
subsequence against those incremented values.
So the parallel overloads only found subsequences of consecutive values
such as {1, 2}, which is what the existing tests use. For other
subsequences they missed matches or reported false ones, and they did
not compile for element types without operator++, such as std::string.
Keep an iterator in step with the current element, as the parallel
search does, and compare the rest of the subsequence through it. Add a
test with a non-consecutive subsequence, a lone first element that does
not start a match, a match at the very end, and std::string elements.
Signed-off-by: Ishaan400 <[email protected]>
d1f516f to
d9fe428
Compare
Restore the release notes update that was lost when release-2.0.X was rebuilt on master eaff5b9, and describe the PRs merged since the previous rebuild (#6514, #7767, #7816, #7817, #7819, #7821, #7824, #7831). Regenerate the closed issue and pull request lists for release-2.0.X at 4cb902e against the v1.11.0 tag (775 PRs, 167 issues). Signed-off-by: Hartmut Kaiser <[email protected]>
Fixes #7766
Proposed Changes
sequential_find_end_t, keep an iterator (curr) in step with the element thatutil::loop_idx_npasses to the callback, and compare the rest of the subsequence through it (*mid). Before, the callback incremented a copy of the element itself, so the rest of the subsequence was compared witht + 1,t + 2, ... This is the same approach the parallelsearchalready uses indetail/search.hpp.std::iterator_traits<Iter1>::referenceinstead ofauto, so it is not copied.test_find_end5to thefindendunit test forseq,parandpar_unseqwith random access and forward iterators. It uses the subsequence{2, 1}in the middle of the vector, followed by a lone2that must not be taken for a match, then again at the very end, and finally the same data asstd::strings.Any background context you want to provide?
The existing
findendtests only use{1, 2}andstd::iotasubsequences, wheret + 1happens to be the next element of the subsequence, so they passed. Any other subsequence could be missed or matched falsely ({1, 5}matched{1, 2}), and element types withoutoperator++such asstd::stringdid not compile.Testing (GCC 13.3, Ubuntu 24.04, local-only build):
findend,findend_sender,find_end_range,find_end_range2andminimal_findendpass,findendalso with--hpx:threads=1.std::stringpart of the new test fails to compile (no match for 'operator++'), and without that part 8 of the new checks fail.hpx::find_endwithpar,par_unseq,par(task),hpx::ranges::find_end(with and without projections),std::forward_listandstd::stringelements againststd::find_endon 400 random inputs of up to 20000 elements: 2800 of 2800 identical, against 813 of 2400 on master (thestd::stringcase excluded there, as it does not compile).clang-format-20makes no changes.Checklist
Not all points below apply to all pull requests.