Details
- Reviewers
jdoerfert var-const - Group Reviewers
Restricted Project - Commits
- rG8a61749f767e: [libc++][ranges] implement `std::ranges::inplace_merge`
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Thanks a lot! A few nitpicks, and let's discuss algorithm_family.h offline (I'll post a summary here afterwards).
libcxx/docs/Status/RangesAlgorithms.csv | ||
---|---|---|
80 | The patch number needs to be updated. | |
libcxx/include/__algorithm/inplace_merge.h | ||
50 | Nit: it's best not to do these refactoring changes in this patch (for the lines that you don't otherwise modify). It increases the delta that needs to be reviewed, but more importantly, it makes it harder to navigate git blame. Ideally, such changes should go to dedicated refactoring patches which can then be added to .git-blame-ignore-revs. This doesn't apply to the lines that you're modifying anyway -- since those would show up in git blame regardless, might as well refactor them. | |
libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_inplace_merge.pass.cpp | ||
318 | Nit: "yet" makes it a little ambiguous whether it's not constexpr in the standard or just in our implementation. I'd rephrase to inplace_merge is not constexpr in the latest finished Standard. | |
329 | Optional: consider asserting that (input.begin(), input.begin() + midIdx) and (input.begin() + midIdx, input.end()) are is_sorted. | |
360 | Missing [first, mid). | |
381 | Nit: s/duplictes/duplicates/. | |
402 | Consider also checking mid == first + 1 and mid == last - 1 (to check for off-by-one errors). | |
510 | Please copy the specification from the standard into this comment. | |
529 | Ultranits: s/specify/specifies/ and s/does not/don't/. | |
529 | I noticed the same with shuffle. It could be a simple oversight in the spec. |
libcxx/include/__algorithm/algorithm_family.h | ||
---|---|---|
26 | Nit: since we're using _AlgPolicy, this should be _AlgFamily (no o). |
The patch number needs to be updated.