Page MenuHomePhabricator
Differential D101737 Diff 349064 libcxx/include/__ranges/view_interface.h

Changeset View

Standalone View

View Options

libcxx/include/__ranges/view_interface.h

  • This file was added.
// -*- C++ -*-
//===----------------------------------------------------------------------===//
//
// 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
//
//===----------------------------------------------------------------------===//
#ifndef _LIBCPP___RANGES_VIEW_INTERFACE_H
#define _LIBCPP___RANGES_VIEW_INTERFACE_H
#include <__config>
#include <__iterator/concepts.h>
#include <__iterator/iterator_traits.h>
#include <__iterator/prev.h>
#include <__ranges/access.h>
#include <__ranges/empty.h>
#include <__ranges/view.h>
#include <type_traits>
#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#pragma GCC system_header
#endif
_LIBCPP_PUSH_MACROS
#include <__undef_macros>
_LIBCPP_BEGIN_NAMESPACE_STD
#if !defined(_LIBCPP_HAS_NO_RANGES)
namespace ranges {
template<class _Tp>
concept __can_empty = requires(_Tp __t) { ranges::empty(__t); };
template<class _Tp>
void __implicitly_convert_to(type_identity_t<_Tp>) noexcept;
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

I will remove this once the patch lands and I rebase.

zoecarver: I will remove this once the patch lands and I rebase.
cjdbUnsubmitted
Not Done
ReplyInline Actions

It doesn't look like the concept is used in your patch anyway?

cjdb: It doesn't look like the concept is used in your patch anyway?
ldionneUnsubmitted
Done
ReplyInline Actions

Can drop nodiscard.

ldionne: Can drop `nodiscard`.
template<class _Derived>
requires is_class_v<_Derived> && same_as<_Derived, remove_cv_t<_Derived>>
class view_interface : public view_base {
constexpr _Derived& __derived() noexcept {
return static_cast<_Derived&>(*this);
}
cjdbUnsubmitted
Not Done
ReplyInline Actions

Please make this a reference.

cjdb: Please make this a reference.
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

In this case T = Derived so I don't think it matters. I've added a test where *this is of type move only rvalue.

zoecarver: In this case T = Derived so I don't think it matters. I've added a test where `*this` is of…
constexpr _Derived const& __derived() const noexcept {
return static_cast<_Derived const&>(*this);
}
CaseyCarterUnsubmitted
Done
ReplyInline Actions

You're missing that the implicit conversion to of this expression to bool could be potentially throwing. We use something like:

c++
template <class T>
void implicit_convert_to(type_identity_t<T>) noexcept;
/* ... */
  noexcept(noexcept(implicit_convert_to<bool>(ranges::begin(__derived()) == ranges::end(__derived()))))
CaseyCarter: You're missing that the implicit conversion to of this expression to `bool` could be…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Done and added a regression test.

zoecarver: Done and added a regression test.
public:
template<class _D2 = _Derived>
[[nodiscard]] constexpr bool empty()
noexcept(noexcept(__implicitly_convert_to<bool>(ranges::begin(__derived()) == ranges::end(__derived()))))
requires forward_range<_D2>
{
ldionneUnsubmitted
Done
ReplyInline Actions

Why is that necessary? Is that the workaround for gcc -fconcepts you're discussing in the comments below?

ldionne: Why is that necessary? Is that the workaround for `gcc -fconcepts` you're discussing in the…
CaseyCarterUnsubmitted
Done
ReplyInline Actions

This is necessary to workaround https://bugs.llvm.org/show_bug.cgi?id=44833.

CaseyCarter: This is necessary to workaround https://bugs.llvm.org/show_bug.cgi?id=44833.
ldionneUnsubmitted
Not Done
ReplyInline Actions

Thanks!

ldionne: Thanks!
return ranges::begin(__derived()) == ranges::end(__derived());
}
template<class _D2 = _Derived>
cjdbUnsubmitted
Not Done
ReplyInline Actions

Oh, this issue. I remember when gcc -fconcepts had this problem back in the day :(

cjdb: Oh, //this// issue. I remember when `gcc -fconcepts` had this problem back in the day :(
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

:(

Also, I couldn't do __can_empty inline.

And if you don't have this, it fails in a very confusing way. Spent a lot of time figuring out why view_interface sort of thought _Derived was an incomplete type but could call members on the type. I finally figured it out when I discovered that it only appeared to be an incomplete type when evaluating the constraints. I suspect clang is simply forgetting a record = record->getDefinition() somewhere.

zoecarver: :( Also, I couldn't do `__can_empty` inline. And if you don't have this, it fails in a very…
[[nodiscard]] constexpr bool empty() const
noexcept(noexcept(__implicitly_convert_to<bool>(ranges::begin(__derived()) == ranges::end(__derived()))))
requires forward_range<const _D2>
{
return ranges::begin(__derived()) == ranges::end(__derived());
}
template<class _D2 = _Derived>
constexpr explicit operator bool()
noexcept(noexcept(ranges::empty(declval<_D2>())))
requires __can_empty<_D2>
{
return !ranges::empty(__derived());
}
template<class _D2 = _Derived>
constexpr explicit operator bool() const
noexcept(noexcept(ranges::empty(declval<const _D2>())))
requires __can_empty<const _D2>
{
return !ranges::empty(__derived());
}
template<class _D2 = _Derived>
constexpr auto data()
noexcept(noexcept(_VSTD::to_address(ranges::begin(__derived()))))
requires contiguous_iterator<iterator_t<_D2>>
{
return _VSTD::to_address(ranges::begin(__derived()));
}
template<class _D2 = _Derived>
constexpr auto data() const
noexcept(noexcept(_VSTD::to_address(ranges::begin(__derived()))))
requires range<const _D2> && contiguous_iterator<iterator_t<const _D2>>
{
return _VSTD::to_address(ranges::begin(__derived()));
}
template<class _D2 = _Derived>
constexpr auto size()
noexcept(noexcept(ranges::end(__derived()) - ranges::begin(__derived())))
requires forward_range<_D2>
&& sized_sentinel_for<sentinel_t<_D2>, iterator_t<_D2>>
{
return ranges::end(__derived()) - ranges::begin(__derived());
}
template<class _D2 = _Derived>
constexpr auto size() const
noexcept(noexcept(ranges::end(__derived()) - ranges::begin(__derived())))
requires forward_range<const _D2>
&& sized_sentinel_for<sentinel_t<const _D2>, iterator_t<const _D2>>
{
return ranges::end(__derived()) - ranges::begin(__derived());
}
template<class _D2 = _Derived>
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Remove all the nodiscards except for empty.

zoecarver: Remove all the nodiscards except for empty.
QuuxplusoneUnsubmitted
Not Done
ReplyInline Actions

It would be appropriate to mark them _LIBCPP_NODISCARD_EXT (with tests).

Quuxplusone: It would be appropriate to mark them `_LIBCPP_NODISCARD_EXT` (with tests).
cjdbUnsubmitted
Not Done
ReplyInline Actions

Remove all the nodiscards except for empty.

Please elaborate.

It would be appropriate to mark them _LIBCPP_NODISCARD_EXT (with tests).

Why is this different to [[nodiscard]] + disabling the warning?

cjdb: > Remove all the nodiscards except for empty. Please elaborate. > It would be appropriate to…
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Please elaborate.

We've discussed elsewhere, I think libc++ is only going to apply nodiscard to methods where a user might think that a side effect is happening (such as .empty()).

zoecarver: > Please elaborate. We've discussed elsewhere, I think libc++ is only going to apply nodiscard…
constexpr decltype(auto) front()
noexcept(noexcept(*ranges::begin(__derived())))
requires forward_range<_D2>
{
_LIBCPP_ASSERT(!empty(),
"Precondition `!empty()` not satisfied. `.front()` called on an empty view.");
return *ranges::begin(__derived());
}
template<class _D2 = _Derived>
constexpr decltype(auto) front() const
noexcept(noexcept(*ranges::begin(__derived())))
requires forward_range<const _D2>
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Add a message here.

zoecarver: Add a message here.
{
_LIBCPP_ASSERT(!empty(),
"Precondition `!empty()` not satisfied. `.front()` called on an empty view.");
return *ranges::begin(__derived());
}
template<class _D2 = _Derived>
constexpr decltype(auto) back()
noexcept(noexcept(*ranges::prev(ranges::end(__derived()))))
requires bidirectional_range<_D2> && common_range<_D2>
{
_LIBCPP_ASSERT(!empty(),
"Precondition `!empty()` not satisfied. `.back()` called on an empty view.");
return *ranges::prev(ranges::end(__derived()));
}
template<class _D2 = _Derived>
constexpr decltype(auto) back() const
noexcept(noexcept(*ranges::prev(ranges::end(__derived()))))
zoecarverAuthorUnsubmitted
Done
ReplyInline Actions

Review note: this is used until ranges::prev is implemented.

zoecarver: Review note: this is used until `ranges::prev` is implemented.
cjdbUnsubmitted
Done
ReplyInline Actions

Thanks for highlighting this, was about to comment.

cjdb: Thanks for highlighting this, was about to comment.
ldionneUnsubmitted
Done
ReplyInline Actions

This can be changed now (and also in the noexcept(...) above).

ldionne: This can be changed now (and also in the `noexcept(...)` above).
requires bidirectional_range<const _D2> && common_range<const _D2>
{
_LIBCPP_ASSERT(!empty(),
"Precondition `!empty()` not satisfied. `.back()` called on an empty view.");
return *ranges::prev(ranges::end(__derived()));
}
template<random_access_range _RARange = _Derived>
constexpr decltype(auto) operator[](range_difference_t<_RARange> __index)
noexcept(noexcept(ranges::begin(__derived())[__index]))
{
return ranges::begin(__derived())[__index];
}
template<random_access_range _RARange = const _Derived>
constexpr decltype(auto) operator[](range_difference_t<_RARange> __index) const
noexcept(noexcept(ranges::begin(__derived())[__index]))
{
return ranges::begin(__derived())[__index];
}
};
}
#endif // !defined(_LIBCPP_HAS_NO_RANGES)
_LIBCPP_END_NAMESPACE_STD
_LIBCPP_POP_MACROS
#endif // _LIBCPP___RANGES_VIEW_INTERFACE_H