Skip to content
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.

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

[libc++] Fix take_view::__sentinel's operator== #74655

Merged
merged 16 commits into from
Dec 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions libcxx/include/__ranges/take_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -180,10 +180,9 @@ class take_view<_View>::__sentinel {
return __lhs.count() == 0 || __lhs.base() == __rhs.__end_;
}

template<bool _OtherConst = !_Const>
template <bool _OtherConst = !_Const>
requires sentinel_for<sentinel_t<_Base>, iterator_t<__maybe_const<_OtherConst, _View>>>
_LIBCPP_HIDE_FROM_ABI
friend constexpr bool operator==(const _Iter<_Const>& __lhs, const __sentinel& __rhs) {
_LIBCPP_HIDE_FROM_ABI friend constexpr bool operator==(const _Iter<_OtherConst>& __lhs, const __sentinel& __rhs) {
return __lhs.count() == 0 || __lhs.base() == __rhs.__end_;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing to do with your patch, just wanted to point out that we don’t seem to have tests to test the default argument. (you don’t need to do it as this is nothing to do with the bug you are fixing)
template

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried changing default argument to _Const and removing it, and in both cases test failed. I think that current coverage is enough.

};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@

// UNSUPPORTED: c++03, c++11, c++14, c++17

// sentinel() = default;
// constexpr explicit sentinel(sentinel_t<Base> end);
// constexpr sentinel(sentinel<!Const> s)
// requires Const && convertible_to<sentinel_t<V>, sentinel_t<Base>>;
// constexpr sentinel_t<Base> base() const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not attached to this line. What is the reason to rename the folder? The folder name is meant to itch the spec section id

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And range.take.sentinel is correct name of the spec section: http://eel.is/c++draft/range.take.sentinel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that's already encoded in the parent directory as all other tests use just "sentinel" and "iterator"


#include <ranges>
#include <cassert>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@ constexpr bool test() {

{
// Test the conversion from "sentinel" to "sentinel-to-const".
using TakeView = std::ranges::take_view<MoveOnlyView>;
using Sentinel = std::ranges::sentinel_t<TakeView>;
using TakeView = std::ranges::take_view<MoveOnlyView>;
using Sentinel = std::ranges::sentinel_t<TakeView>;
using ConstSentinel = std::ranges::sentinel_t<const TakeView>;
static_assert(std::is_convertible_v<Sentinel, ConstSentinel>);
TakeView tv = TakeView(MoveOnlyView(buffer), 4);
Sentinel s = tv.end();
TakeView tv = TakeView(MoveOnlyView(buffer), 4);
Sentinel s = tv.end();
ConstSentinel cs = s;
cs = s; // test assignment also
cs = s; // test assignment also
assert(tv.begin() + 4 == s);
assert(tv.begin() + 4 == cs);
assert(std::as_const(tv).begin() + 4 == s);
Expand All @@ -50,12 +50,12 @@ constexpr bool test() {

{
// Test the constructor from "base-sentinel" to "sentinel".
using TakeView = std::ranges::take_view<MoveOnlyView>;
using Sentinel = std::ranges::sentinel_t<TakeView>;
using TakeView = std::ranges::take_view<MoveOnlyView>;
using Sentinel = std::ranges::sentinel_t<TakeView>;
sentinel_wrapper<int*> sw1 = MoveOnlyView(buffer).end();
static_assert( std::is_constructible_v<Sentinel, sentinel_wrapper<int*>>);
static_assert(std::is_constructible_v<Sentinel, sentinel_wrapper<int*>>);
static_assert(!std::is_convertible_v<sentinel_wrapper<int*>, Sentinel>);
auto s = Sentinel(sw1);
auto s = Sentinel(sw1);
std::same_as<sentinel_wrapper<int*>> auto sw2 = s.base();
assert(base(sw2) == base(sw1));
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
//===----------------------------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

// UNSUPPORTED: c++03, c++11, c++14, c++17

// friend constexpr bool operator==(const CI<Const>& y, const sentinel& x);
// template<bool OtherConst = !Const>
// requires sentinel_for<sentinel_t<Base>, iterator_t<maybe-const<OtherConst, V>>>
// friend constexpr bool operator==(const CI<OtherConst>& y, const sentinel& x);

#include <cassert>
#include <cstddef>
#include <ranges>
#include <type_traits>
#include <utility>

#include "test_comparisons.h"
#include "test_iterators.h"

template <bool Const>
using MaybeConstIterator = cpp20_input_iterator<std::conditional_t<Const, const int*, int*>>;

template <bool Const>
class CrossConstComparableSentinel {
using Base = std::conditional_t<Const, const int*, int*>;
Base base_;

public:
CrossConstComparableSentinel() = default;
constexpr explicit CrossConstComparableSentinel(Base base) : base_(base) {}

friend constexpr bool operator==(const MaybeConstIterator<Const>& it, const CrossConstComparableSentinel& se) {
return base(it) == se.base_;
}

friend constexpr bool operator==(const MaybeConstIterator<!Const>& it, const CrossConstComparableSentinel& se) {
return base(it) == se.base_;
}
};

static_assert(std::sentinel_for<CrossConstComparableSentinel<true>, MaybeConstIterator<false>>);
static_assert(std::sentinel_for<CrossConstComparableSentinel<true>, MaybeConstIterator<true>>);
static_assert(std::sentinel_for<CrossConstComparableSentinel<false>, MaybeConstIterator<false>>);
static_assert(std::sentinel_for<CrossConstComparableSentinel<false>, MaybeConstIterator<true>>);

struct CrossConstComparableView : std::ranges::view_base {
template <std::size_t N>
constexpr explicit CrossConstComparableView(int (&arr)[N]) : b_(arr), e_(arr + N) {}

constexpr MaybeConstIterator<false> begin() { return MaybeConstIterator<false>{b_}; }
constexpr CrossConstComparableSentinel<false> end() { return CrossConstComparableSentinel<false>{e_}; }

constexpr MaybeConstIterator<true> begin() const { return MaybeConstIterator<true>{b_}; }
constexpr CrossConstComparableSentinel<true> end() const { return CrossConstComparableSentinel<true>{e_}; }

private:
int* b_;
int* e_;
};

static_assert(std::ranges::range<CrossConstComparableView>);
static_assert(std::ranges::range<const CrossConstComparableView>);

struct NonCrossConstComparableView : std::ranges::view_base {
int* begin();
sentinel_wrapper<int*> end();

long* begin() const;
sentinel_wrapper<long*> end() const;
};

static_assert(std::ranges::range<NonCrossConstComparableView>);
static_assert(std::ranges::range<const NonCrossConstComparableView>);

template <class T, class U>
concept weakly_equality_comparable_with = requires(const T& t, const U& u) {
t == u;
t != u;
u == t;
u != t;
};

constexpr bool test() {
int buffer[8] = {1, 2, 3, 4, 5, 6, 7, 8};
using CrossConstComparableTakeView = std::ranges::take_view<CrossConstComparableView>;

{ // Compare CI<Const> with sentinel<Const>
{ // Const == true
AssertEqualityReturnBool<std::ranges::iterator_t<const CrossConstComparableTakeView>,
std::ranges::sentinel_t<const CrossConstComparableTakeView>>();
const CrossConstComparableTakeView tv(CrossConstComparableView{buffer}, 4);
assert(testEquality(std::ranges::next(tv.begin(), 4), tv.end(), true));
assert(testEquality(tv.begin(), tv.end(), false));
}

{ // Const == false
AssertEqualityReturnBool<std::ranges::iterator_t<CrossConstComparableTakeView>,
std::ranges::sentinel_t<CrossConstComparableTakeView>>();
CrossConstComparableTakeView tv(CrossConstComparableView{buffer}, 4);
assert(testEquality(std::ranges::next(tv.begin(), 4), tv.end(), true));
assert(testEquality(std::ranges::next(tv.begin(), 1), tv.end(), false));
}
}

{ // Compare CI<Const> with sentinel<!Const>
{ // Const == true
AssertEqualityReturnBool<std::ranges::iterator_t<const CrossConstComparableTakeView>,
std::ranges::sentinel_t<CrossConstComparableTakeView>>();
CrossConstComparableTakeView tv(CrossConstComparableView{buffer}, 4);
assert(testEquality(std::ranges::next(std::as_const(tv).begin(), 4), tv.end(), true));
assert(testEquality(std::ranges::next(std::as_const(tv).begin(), 2), tv.end(), false));
}

{ // Const == false
AssertEqualityReturnBool<std::ranges::iterator_t<CrossConstComparableTakeView>,
std::ranges::sentinel_t<const CrossConstComparableTakeView>>();
CrossConstComparableTakeView tv(CrossConstComparableView{buffer}, 4);
assert(testEquality(std::ranges::next(tv.begin(), 4), std::as_const(tv).end(), true));
assert(testEquality(std::ranges::next(tv.begin(), 3), std::as_const(tv).end(), false));
}
}

{ // Check invalid comparisons between CI<Const> and sentinel<!Const>
using TakeView = std::ranges::take_view<NonCrossConstComparableView>;
static_assert(
!weakly_equality_comparable_with<std::ranges::iterator_t<const TakeView>, std::ranges::sentinel_t<TakeView>>);
static_assert(
!weakly_equality_comparable_with<std::ranges::iterator_t<TakeView>, std::ranges::sentinel_t<const TakeView>>);

// Those should be valid
static_assert(
weakly_equality_comparable_with<std::ranges::iterator_t<TakeView>, std::ranges::sentinel_t<TakeView>>);
static_assert(weakly_equality_comparable_with<std::ranges::iterator_t<const TakeView>,
std::ranges::sentinel_t<const TakeView>>);
}

return true;
}

int main(int, char**) {
test();
static_assert(test());

return 0;
}

This file was deleted.

8 changes: 3 additions & 5 deletions libcxx/test/support/test_comparisons.h
Original file line number Diff line number Diff line change
Expand Up @@ -213,13 +213,11 @@ void AssertEqualityAreNoexcept()
}

template <class T, class U = T>
void AssertEqualityReturnBool()
{
ASSERT_SAME_TYPE(decltype(std::declval<const T&>() == std::declval<const U&>()), bool);
ASSERT_SAME_TYPE(decltype(std::declval<const T&>() != std::declval<const U&>()), bool);
TEST_CONSTEXPR_CXX14 void AssertEqualityReturnBool() {
ASSERT_SAME_TYPE(decltype(std::declval<const T&>() == std::declval<const U&>()), bool);
ASSERT_SAME_TYPE(decltype(std::declval<const T&>() != std::declval<const U&>()), bool);
}


template <class T, class U = T>
void AssertEqualityConvertibleToBool()
{
Expand Down