Skip to content

partial_sort_copy bugfix #1088

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

Merged
StephanTLavavej merged 5 commits into from
Aug 9, 2020
Merged

partial_sort_copy bugfix #1088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
46dd1c7
partial_sort_copy bugfix
CaseyCarter Jul 25, 2020
f0f9857
STL's review comments
CaseyCarter Jul 26, 2020
d5e542d
Merge branch 'master' into partial_sort_copy_fix
CaseyCarter Jul 27, 2020
d67a7d0
Address final? review comments
CaseyCarter Jul 28, 2020
c1336c8
Use dst_size
StephanTLavavej Jul 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 71 additions & 62 deletions stl/inc/algorithm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5794,7 +5794,7 @@ _CONSTEXPR20 void _Push_heap_by_index(
_Hole = _Idx;
}

*(_First + _Hole) = _STD move(_Val); // drop _Val into final hole
*(_First + _Hole) = _STD forward<_Ty>(_Val); // drop _Val into final hole
}

template <class _RanIt, class _Pr>
Expand All @@ -5820,16 +5820,17 @@ _CONSTEXPR20 void push_heap(_RanIt _First, _RanIt _Last) {
#ifdef __cpp_lib_concepts
namespace ranges {
// VARIABLE ranges::push_heap
template <class _It, class _Pr, class _Pj>
// clang-format off
template <random_access_iterator _It, class _Ty, class _Pr, class _Pj1, class _Pj2>
requires sortable<_It, _Pr, _Pj1> && indirectly_writable<_It, _Ty>
&& indirect_strict_weak_order<_Pr, projected<_It, _Pj1>, projected<remove_reference_t<_Ty>*, _Pj2>>
constexpr void _Push_heap_by_index(const _It _First, iter_difference_t<_It> _Hole,
const iter_difference_t<_It> _Top, iter_value_t<_It>&& _Val, _Pr _Pred, _Pj _Proj) {
const iter_difference_t<_It> _Top, _Ty&& _Val, _Pr _Pred, _Pj1 _Proj1, _Pj2 _Proj2) {
// clang-format on
// percolate _Hole to _Top or where _Val belongs
_STL_INTERNAL_STATIC_ASSERT(random_access_iterator<_It>);
_STL_INTERNAL_STATIC_ASSERT(sortable<_It, _Pr, _Pj>);

while (_Top < _Hole) {
const auto _Idx = static_cast<iter_difference_t<_It>>((_Hole - 1) >> 1); // shift for codegen
if (!_STD invoke(_Pred, _STD invoke(_Proj, *(_First + _Idx)), _STD invoke(_Proj, _Val))) {
if (!_STD invoke(_Pred, _STD invoke(_Proj1, *(_First + _Idx)), _STD invoke(_Proj2, _Val))) {
break;
}

Expand All @@ -5838,7 +5839,7 @@ namespace ranges {
_Hole = _Idx;
}

*(_First + _Hole) = _STD move(_Val); // drop _Val into final hole
*(_First + _Hole) = _STD forward<_Ty>(_Val); // drop _Val into final hole
}

class _Push_heap_fn : private _Not_quite_object {
Expand Down Expand Up @@ -5883,7 +5884,8 @@ namespace ranges {

--_Last;
iter_value_t<_It> _Val = _RANGES iter_move(_Last);
_RANGES _Push_heap_by_index(_STD move(_First), _Count - 1, 0, _STD move(_Val), _Pred, _Proj);
// NB: if _Proj is a _Ref_fn, this aliases the _Proj1 and _Proj2 parameters of _Push_heap_by_index
_RANGES _Push_heap_by_index(_STD move(_First), _Count - 1, 0, _STD move(_Val), _Pred, _Proj, _Proj);
}
};

Expand Down Expand Up @@ -5919,7 +5921,7 @@ _CONSTEXPR20 void _Pop_heap_hole_by_index(
_Hole = _Bottom - 1;
}

_Push_heap_by_index(_First, _Hole, _Top, _STD move(_Val), _Pred);
_Push_heap_by_index(_First, _Hole, _Top, _STD forward<_Ty>(_Val), _Pred);
}

template <class _RanIt, class _Ty, class _Pr>
Expand All @@ -5929,7 +5931,8 @@ _CONSTEXPR20 void _Pop_heap_hole_unchecked(_RanIt _First, _RanIt _Last, _RanIt _
// precondition: _First != _Dest
*_Dest = _STD move(*_First);
using _Diff = _Iter_diff_t<_RanIt>;
_Pop_heap_hole_by_index(_First, static_cast<_Diff>(0), static_cast<_Diff>(_Last - _First), _STD move(_Val), _Pred);
_Pop_heap_hole_by_index(
_First, static_cast<_Diff>(0), static_cast<_Diff>(_Last - _First), _STD forward<_Ty>(_Val), _Pred);
}

template <class _RanIt, class _Pr>
Expand Down Expand Up @@ -5958,12 +5961,14 @@ _CONSTEXPR20 void pop_heap(_RanIt _First, _RanIt _Last) {
#ifdef __cpp_lib_concepts
namespace ranges {
// VARIABLE ranges::pop_heap
template <class _It, class _Pr, class _Pj>
// clang-format off
template <random_access_iterator _It, class _Ty, class _Pr, class _Pj1, class _Pj2>
requires sortable<_It, _Pr, _Pj1> && indirectly_writable<_It, _Ty>
&& indirect_strict_weak_order<_Pr, projected<_It, _Pj1>, projected<remove_reference_t<_Ty>*, _Pj2>>
constexpr void _Pop_heap_hole_by_index(_It _First, iter_difference_t<_It> _Hole,
const iter_difference_t<_It> _Bottom, iter_value_t<_It>&& _Val, _Pr _Pred, _Pj _Proj) {
const iter_difference_t<_It> _Bottom, _Ty&& _Val, _Pr _Pred, _Pj1 _Proj1, _Pj2 _Proj2) {
// clang-format on
// percolate _Hole to _Bottom, then push _Val
_STL_INTERNAL_STATIC_ASSERT(random_access_iterator<_It>);
_STL_INTERNAL_STATIC_ASSERT(sortable<_It, _Pr, _Pj>);
_STL_INTERNAL_CHECK(_Hole >= 0);
_STL_INTERNAL_CHECK(_Bottom > 0);

Expand All @@ -5977,7 +5982,7 @@ namespace ranges {
while (_Idx < _Max_sequence_non_leaf) { // move _Hole down to larger child
_Idx = 2 * _Idx + 2;
auto _Mid = _First + _Idx;
if (_STD invoke(_Pred, _STD invoke(_Proj, *_Mid), _STD invoke(_Proj, *_RANGES prev(_Mid)))) {
if (_STD invoke(_Pred, _STD invoke(_Proj1, *_Mid), _STD invoke(_Proj1, *_RANGES prev(_Mid)))) {
--_Idx;
--_Mid;
}
Expand All @@ -5990,36 +5995,39 @@ namespace ranges {
_Hole = _Bottom - 1;
}

_RANGES _Push_heap_by_index(_STD move(_First), _Hole, _Top, _STD move(_Val), _Pred, _Proj);
_RANGES _Push_heap_by_index(_STD move(_First), _Hole, _Top, _STD forward<_Ty>(_Val), _Pred, _Proj1, _Proj2);
}

template <class _It, class _Pr, class _Pj>
// clang-format off
template <random_access_iterator _It, class _Ty, class _Pr, class _Pj1, class _Pj2>
requires sortable<_It, _Pr, _Pj1> && indirectly_writable<_It, _Ty>
&& indirect_strict_weak_order<_Pr, projected<_It, _Pj1>, projected<remove_reference_t<_Ty>*, _Pj2>>
constexpr void _Pop_heap_hole_unchecked(
_It _First, const _It _Last, const _It _Dest, iter_value_t<_It>&& _Val, _Pr _Pred, _Pj _Proj) {
_It _First, const _It _Last, const _It _Dest, _Ty&& _Val, _Pr _Pred, _Pj1 _Proj1, _Pj2 _Proj2) {
// clang-format on
// pop *_First to *_Dest and reheap
_STL_INTERNAL_STATIC_ASSERT(random_access_iterator<_It>);
_STL_INTERNAL_STATIC_ASSERT(sortable<_It, _Pr, _Pj>);
_STL_INTERNAL_CHECK(_First != _Last);
_STL_INTERNAL_CHECK(_First != _Dest);

*_Dest = _RANGES iter_move(_First);
const auto _Count = _Last - _First;
_RANGES _Pop_heap_hole_by_index(_STD move(_First), 0, _Count, _STD move(_Val), _Pred, _Proj);
_RANGES _Pop_heap_hole_by_index(_STD move(_First), 0, _Count, _STD forward<_Ty>(_Val), _Pred, _Proj1, _Proj2);
}

template <class _It, class _Pr, class _Pj>
// clang-format off
template <random_access_iterator _It, class _Pr, class _Pj>
requires sortable<_It, _Pr, _Pj>
constexpr void _Pop_heap_unchecked(_It _First, _It _Last, _Pr _Pred, _Pj _Proj) {
// clang-format on
// pop *_First to *(_Last - 1) and reheap
_STL_INTERNAL_STATIC_ASSERT(random_access_iterator<_It>);
_STL_INTERNAL_STATIC_ASSERT(sortable<_It, _Pr, _Pj>);

if (_Last - _First < 2) {
return;
}

--_Last;
iter_value_t<_It> _Val = _RANGES iter_move(_Last);
_RANGES _Pop_heap_hole_unchecked(_STD move(_First), _Last, _Last, _STD move(_Val), _Pred, _Proj);
// NB: if _Proj is a _Ref_fn, this aliases the _Proj1 and _Proj2 parameters of _Pop_heap_hole_unchecked
_RANGES _Pop_heap_hole_unchecked(_STD move(_First), _Last, _Last, _STD move(_Val), _Pred, _Proj, _Proj);
}

class _Pop_heap_fn : private _Not_quite_object {
Expand Down Expand Up @@ -6085,6 +6093,23 @@ _CONSTEXPR20 void make_heap(_RanIt _First, _RanIt _Last) { // make [_First, _Las
#ifdef __cpp_lib_concepts
namespace ranges {
// VARIABLE ranges::make_heap
// clang-format off
template <random_access_iterator _It, class _Pr, class _Pj>
requires sortable<_It, _Pr, _Pj>
constexpr void _Make_heap_common(_It _First, _It _Last, _Pr _Pred, _Pj _Proj) {
// make nontrivial [_First, _Last) into a heap with respect to _Pred and _Proj
// clang-format on
using _Diff = iter_difference_t<_It>;
const _Diff _Bottom = _Last - _First;
for (_Diff _Hole = _Bottom >> 1; _Hole > 0;) { // shift for codegen
// reheap top half, bottom to top
--_Hole;
iter_value_t<_It> _Val = _RANGES iter_move(_First + _Hole);
// NB: if _Proj is a _Ref_fn, this aliases the _Proj1 and _Proj2 parameters of _Pop_heap_hole_by_index
_RANGES _Pop_heap_hole_by_index(_First, _Hole, _Bottom, _STD move(_Val), _Pred, _Proj, _Proj);
}
}

class _Make_heap_fn : private _Not_quite_object {
public:
using _Not_quite_object::_Not_quite_object;
Expand All @@ -6097,39 +6122,23 @@ namespace ranges {
auto _UFirst = _Get_unwrapped(_STD move(_First));
auto _ULast = _Get_final_iterator_unwrapped<_It>(_UFirst, _STD move(_Last));
_Seek_wrapped(_First, _ULast);
_Make_heap_unchecked(_STD move(_UFirst), _STD move(_ULast), _Pass_fn(_Pred), _Pass_fn(_Proj));
_Make_heap_common(_STD move(_UFirst), _STD move(_ULast), _Pass_fn(_Pred), _Pass_fn(_Proj));
return _First;
}

template <random_access_range _Rng, class _Pr = ranges::less, class _Pj = identity>
requires sortable<iterator_t<_Rng>, _Pr, _Pj>
constexpr borrowed_iterator_t<_Rng> operator()(_Rng&& _Range, _Pr _Pred = {}, _Pj _Proj = {}) const {
if constexpr (common_range<_Rng>) {
_Make_heap_unchecked(_Ubegin(_Range), _Uend(_Range), _Pass_fn(_Pred), _Pass_fn(_Proj));
_Make_heap_common(_Ubegin(_Range), _Uend(_Range), _Pass_fn(_Pred), _Pass_fn(_Proj));
return _RANGES end(_Range);
} else {
auto _ULast = _Get_final_iterator_unwrapped(_Range);
_Make_heap_unchecked(_Ubegin(_Range), _ULast, _Pass_fn(_Pred), _Pass_fn(_Proj));
_Make_heap_common(_Ubegin(_Range), _ULast, _Pass_fn(_Pred), _Pass_fn(_Proj));
return _Rewrap_iterator(_Range, _STD move(_ULast));
}
}
// clang-format on
private:
template <class _It, class _Pr, class _Pj>
static constexpr void _Make_heap_unchecked(_It _First, _It _Last, _Pr _Pred, _Pj _Proj) {
// make nontrivial [_First, _Last) into a heap
_STL_INTERNAL_STATIC_ASSERT(random_access_iterator<_It>);
_STL_INTERNAL_STATIC_ASSERT(sortable<_It, _Pr, _Pj>);

using _Diff = iter_difference_t<_It>;
const _Diff _Bottom = _Last - _First;
for (_Diff _Hole = _Bottom >> 1; _Hole > 0;) { // shift for codegen
// reheap top half, bottom to top
--_Hole;
iter_value_t<_It> _Val = _RANGES iter_move(_First + _Hole);
_RANGES _Pop_heap_hole_by_index(_First, _Hole, _Bottom, _STD move(_Val), _Pred, _Proj);
}
}
};

inline constexpr _Make_heap_fn make_heap{_Not_quite_object::_Construct_tag{}};
Expand Down Expand Up @@ -6318,6 +6327,17 @@ _CONSTEXPR20 void sort_heap(_RanIt _First, _RanIt _Last) { // order heap by repe
#ifdef __cpp_lib_concepts
namespace ranges {
// VARIABLE ranges::sort_heap
// clang-format off
template <random_access_iterator _It, class _Pr, class _Pj>
requires sortable<_It, _Pr, _Pj>
constexpr void _Sort_heap_common(const _It _First, _It _Last, _Pr _Pred, _Pj _Proj) {
// order heap by repeatedly popping
// clang-format on
for (; _Last - _First >= 2; --_Last) {
_RANGES _Pop_heap_unchecked(_First, _Last, _Pred, _Proj);
}
}

class _Sort_heap_fn : private _Not_quite_object {
public:
using _Not_quite_object::_Not_quite_object;
Expand All @@ -6330,34 +6350,23 @@ namespace ranges {
auto _UFirst = _Get_unwrapped(_STD move(_First));
auto _ULast = _Get_final_iterator_unwrapped<_It>(_UFirst, _STD move(_Last));
_Seek_wrapped(_First, _ULast);
_Sort_heap_unchecked(_STD move(_UFirst), _STD move(_ULast), _Pass_fn(_Pred), _Pass_fn(_Proj));
_Sort_heap_common(_STD move(_UFirst), _STD move(_ULast), _Pass_fn(_Pred), _Pass_fn(_Proj));
return _First;
}

template <random_access_range _Rng, class _Pr = ranges::less, class _Pj = identity>
requires sortable<iterator_t<_Rng>, _Pr, _Pj>
constexpr borrowed_iterator_t<_Rng> operator()(_Rng&& _Range, _Pr _Pred = {}, _Pj _Proj = {}) const {
if constexpr (common_range<_Rng>) {
_Sort_heap_unchecked(_Ubegin(_Range), _Uend(_Range), _Pass_fn(_Pred), _Pass_fn(_Proj));
_Sort_heap_common(_Ubegin(_Range), _Uend(_Range), _Pass_fn(_Pred), _Pass_fn(_Proj));
return _RANGES end(_Range);
} else {
auto _ULast = _Get_final_iterator_unwrapped(_Range);
_Sort_heap_unchecked(_Ubegin(_Range), _ULast, _Pass_fn(_Pred), _Pass_fn(_Proj));
_Sort_heap_common(_Ubegin(_Range), _ULast, _Pass_fn(_Pred), _Pass_fn(_Proj));
return _Rewrap_iterator(_Range, _STD move(_ULast));
}
}
// clang-format on
private:
template <class _It, class _Pr, class _Pj>
static constexpr void _Sort_heap_unchecked(const _It _First, _It _Last, _Pr _Pred, _Pj _Proj) {
// order heap by repeatedly popping
_STL_INTERNAL_STATIC_ASSERT(random_access_iterator<_It>);
_STL_INTERNAL_STATIC_ASSERT(sortable<_It, _Pr, _Pj>);

for (; _Last - _First >= 2; --_Last) {
_RANGES _Pop_heap_unchecked(_First, _Last, _Pred, _Proj);
}
}
};

inline constexpr _Sort_heap_fn sort_heap{_Not_quite_object::_Construct_tag{}};
Expand Down Expand Up @@ -7463,8 +7472,8 @@ _CONSTEXPR20 _RanIt partial_sort_copy(_InIt _First1, _InIt _Last1, _RanIt _First
if (_DEBUG_LT_PRED(_Pred, *_UFirst1, *_UFirst2)) {
// replace top with new largest:
using _Diff = _Iter_diff_t<_RanIt>;
_Pop_heap_hole_by_index(_UFirst2, static_cast<_Diff>(0), static_cast<_Diff>(_UMid2 - _UFirst2),
static_cast<_Iter_value_t<_InIt>>(*_UFirst1), _Pass_fn(_Pred));
_Pop_heap_hole_by_index(
_UFirst2, static_cast<_Diff>(0), static_cast<_Diff>(_UMid2 - _UFirst2), *_UFirst1, _Pass_fn(_Pred));
}
}

Expand Down
1 change: 1 addition & 0 deletions tests/std/test.lst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ tests\GH_000690_overaligned_function
tests\GH_000890_pow_template
tests\GH_001010_filesystem_error_encoding
tests\GH_001017_discrete_distribution_out_of_range
tests\GH_001086_partial_sort_copy
tests\LWG2597_complex_branch_cut
tests\LWG3018_shared_ptr_function
tests\P0024R2_parallel_algorithms_adjacent_difference
Expand Down
4 changes: 4 additions & 0 deletions tests/std/tests/GH_001086_partial_sort_copy/env.lst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Copyright (c) Microsoft Corporation.
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

RUNALL_INCLUDE ..\usual_matrix.lst
64 changes: 64 additions & 0 deletions tests/std/tests/GH_001086_partial_sort_copy/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Copyright (c) Microsoft Corporation.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

// GH-1086: "std::partial_sort_copy performs an unconstrained operation"
// partial_sort_copy was constructing an object of the source range's value type
// from the result of dereferencing an iterator, which is not allowed by the
// specification of the algorithm.

#include <algorithm>
#include <cassert>
#include <iterator>

using namespace std;

struct wrapper {
wrapper() = default;
constexpr explicit wrapper(int i) : x{i} {}

bool operator<(const wrapper& that) const {
return x < that.x;
}

int x;
};

struct source : wrapper {
source() = default;

using wrapper::wrapper;

source(const source&) = delete;
source& operator=(const source&) = delete;
};

struct target : wrapper {
target() = default;

using wrapper::wrapper;

target(target&&) = default;
target& operator=(target&&) = default;

target& operator=(const source& w) {
x = w.x;
return *this;
}
};

int main() {
constexpr int src_size = 4;
source src[src_size];
constexpr int dst_size = 2;
target dst[dst_size];

for (int i = 0; i < src_size; ++i) {
src[i].x = src_size - 1 - i;
}

partial_sort_copy(begin(src), end(src), begin(dst), end(dst));

for (int i = 0; i < dst_size; ++i) {
assert(dst[i].x == i);
}
}