Bug 95578 - std::ranges::copy and std::views::take_while don't want to play together
Summary: std::ranges::copy and std::views::take_while don't want to play together
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 10.1.0
: P3 normal
Target Milestone: 10.2
Assignee: Patrick Palka
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-08 15:11 UTC by gcc-bugs
Modified: 2020-06-11 14:32 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2020-06-09 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description gcc-bugs 2020-06-08 15:11:24 UTC
Hi gcc-team,

the following code does not compile (10.1.0 and trunk)

```c++
#include <ranges>
#include <algorithm>

int main()
{
    std::vector<int> v{};
    // auto && rng = v; // does work
    auto && rng = v | std::views::take_while([](auto &&){return true;}); // does not work

    std::vector<int> rng_copy{};
    std::ranges::copy(rng, std::back_inserter(rng_copy));
    return 0;
}
```
https://godbolt.org/z/nmRQgo
Comment 1 Patrick Palka 2020-06-09 03:31:05 UTC
Confirmed.  When writing the normal/reverse/move iterator optimizations for the ranges algos, I wrongly assumed that if the begin() of a range is a normal_iterator (or reverse_iterator, or move_iterator) then the end() must be, too.  But as this testcase shows, it's perfectly valid to have a range whose begin() is a normal_iterator but its end() is not.  So these optimizations should check that the iterator and sentinel have the same type before proceeding.
Comment 2 GCC Commits 2020-06-10 22:01:08 UTC
The master branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

https://gcc.gnu.org/g:a73051a0ea9ce8281e748a74dd924a6eb8fb3723

commit r11-1186-ga73051a0ea9ce8281e748a74dd924a6eb8fb3723
Author: Patrick Palka <ppalka@redhat.com>
Date:   Wed Jun 10 17:37:53 2020 -0400

    libstdc++: Fix some ranges algos optimizations [PR95578]
    
    ranges::copy and a number of other ranges algorithms have unwrapping
    optimizations for iterators of type __normal_iterator, move_iterator and
    reverse_iterator.  But in the checks that guard these optimizations we
    currently only test that the iterator of the iterator/sentinel pair has
    the appropriate type before proceeding with the corresponding
    optimization, and do not also test the sentinel type.
    
    This breaks the testcase in this PR because this testcase constructs via
    range adaptors a range whose begin() is a __normal_iterator and whose
    end() is a custom sentinel type, and then performs ranges::copy on it.
    From there we bogusly perform the __normal_iterator unwrapping
    optimization on this iterator/sentinel pair, which immediately leads to
    a constraint failure since the custom sentinel type does not model
    sentinel_for<int*>.
    
    This patch fixes this issue by refining each of the problematic checks
    to also test that the iterator and sentinel types are the same before
    applying the corresponding unwrapping optimization.  Along the way, some
    code simplifications are made.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/95578
            * include/bits/ranges_algo.h (__lexicographical_compare_fn):
            Also check that the iterator and sentinel have the same type before
            applying the unwrapping optimization for __normal_iterator.
            Split the check into two, one for the first iterator/sentinel
            pair and another for second iterator/sentinel pair.  Remove uses
            of __niter_base, and remove uses of std::move on a
            __normal_iterator.
            * include/bits/ranges_algobase.h (__equal_fn): Likewise.
            (__copy_or_move): Likewise.  Perform similar adjustments for
            the reverse_iterator and move_iterator optimizations.  Inline
            the checks into the if-constexprs, and use using-declarations to
            make them less visually noisy.  Remove uses of __niter_wrap.
            (__copy_or_move_backward): Likewise.
            * testsuite/25_algorithms/copy/95578.cc: New test.
            * testsuite/25_algorithms/copy_backward/95578.cc: New test.
            * testsuite/25_algorithms/equal/95578.cc: New test.
            * testsuite/25_algorithms/lexicographical_compare/95578.cc: New test.
            * testsuite/25_algorithms/move/95578.cc: New test.
            * testsuite/25_algorithms/move_backward/95578.cc: New test.
Comment 3 GCC Commits 2020-06-11 11:54:52 UTC
The releases/gcc-10 branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

https://gcc.gnu.org/g:3e9261f0e013108135bf09f0f91b279b9c5cbd9e

commit r10-8271-g3e9261f0e013108135bf09f0f91b279b9c5cbd9e
Author: Patrick Palka <ppalka@redhat.com>
Date:   Wed Jun 10 17:37:53 2020 -0400

    libstdc++: Fix some ranges algos optimizations [PR95578]
    
    ranges::copy and a number of other ranges algorithms have unwrapping
    optimizations for iterators of type __normal_iterator, move_iterator and
    reverse_iterator.  But in the checks that guard these optimizations we
    currently only test that the iterator of the iterator/sentinel pair has
    the appropriate type before proceeding with the corresponding
    optimization, and do not also test the sentinel type.
    
    This breaks the testcase in this PR because this testcase constructs via
    range adaptors a range whose begin() is a __normal_iterator and whose
    end() is a custom sentinel type, and then performs ranges::copy on it.
    From there we bogusly perform the __normal_iterator unwrapping
    optimization on this iterator/sentinel pair, which immediately leads to
    a constraint failure since the custom sentinel type does not model
    sentinel_for<int*>.
    
    This patch fixes this issue by refining each of the problematic checks
    to also test that the iterator and sentinel types are the same before
    applying the corresponding unwrapping optimization.  Along the way, some
    code simplifications are made.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/95578
            * include/bits/ranges_algo.h (__lexicographical_compare_fn):
            Also check that the iterator and sentinel have the same type before
            applying the unwrapping optimization for __normal_iterator.
            Split the check into two, one for the first iterator/sentinel
            pair and another for second iterator/sentinel pair.  Remove uses
            of __niter_base, and remove uses of std::move on a
            __normal_iterator.
            * include/bits/ranges_algobase.h (__equal_fn): Likewise.
            (__copy_or_move): Likewise.  Perform similar adjustments for
            the reverse_iterator and move_iterator optimizations.  Inline
            the checks into the if-constexprs, and use using-declarations to
            make them less visually noisy.  Remove uses of __niter_wrap.
            (__copy_or_move_backward): Likewise.
            * testsuite/25_algorithms/copy/95578.cc: New test.
            * testsuite/25_algorithms/copy_backward/95578.cc: New test.
            * testsuite/25_algorithms/equal/95578.cc: New test.
            * testsuite/25_algorithms/lexicographical_compare/95578.cc: New test.
            * testsuite/25_algorithms/move/95578.cc: New test.
            * testsuite/25_algorithms/move_backward/95578.cc: New test.
    
    (cherry picked from commit a73051a0ea9ce8281e748a74dd924a6eb8fb3723)
Comment 4 Patrick Palka 2020-06-11 11:55:32 UTC
Fixed for GCC 10.2+, thanks for the bug report.
Comment 5 gcc-bugs 2020-06-11 14:32:34 UTC
Thank you for the quick response and quick fix :)