Conversation
cjdb
left a comment
cjdb
left a comment
There was a problem hiding this comment.
Overall, looking good! There's a few things that need to be addressed on a technical level, as well as a few on a stylistic level.
I've done this review on my phone, so I'll do a more detailed one tomorrow when I can interact with GitHub a bit more nicely.
Thanks for putting this effort in 😄
include/stl2/view/const.hpp
Outdated
|
|
||
| constexpr auto base() const noexcept -> R { return base_; } | ||
|
|
||
| constexpr __iterator<false> begin() |
There was a problem hiding this comment.
You'll need to adda requirement for detail::simple_view on each of the non-const overloads.
There was a problem hiding this comment.
Sorry, I meant !simple_view.
| constexpr auto size() const requires sized_range<R> | ||
| { | ||
| return __stl2::size(base_); | ||
| } |
There was a problem hiding this comment.
A non-const overload is necessary for size.
include/stl2/view/const.hpp
Outdated
|
|
||
| __iterator() = default; | ||
|
|
||
| constexpr explicit __iterator(parent_t &parent) |
include/stl2/view/const.hpp
Outdated
|
|
||
| constexpr void operator++(int) | ||
| { | ||
| current_++; |
include/stl2/view/const.hpp
Outdated
| } | ||
| }; | ||
|
|
||
| template <class R> |
There was a problem hiding this comment.
Please add the requirements here too, as well as viewable_range.
include/stl2/view/const.hpp
Outdated
|
|
||
| namespace views::ext | ||
| { | ||
| struct __as_const_fn : detail::__pipeable<__as_const_fn> |
There was a problem hiding this comment.
Inheritance not needed for this one. Also, please look at some of the other adaptors to see what's usually done. I recommend take or drop.
There was a problem hiding this comment.
I think it's necessary for adaptors with no arguments. same as views::move.
include/stl2/view/const.hpp
Outdated
| template <input_range R> | ||
| requires view<R> | ||
| class const_view : public view_interface<const_view<R>> | ||
| { |
There was a problem hiding this comment.
Please make this
STL2_OPEN_NAMESPACE {
namespace ext {
template<input_range R>
requires view<R>
struct const_view : view_interface<const_view<R>> {There was a problem hiding this comment.
Seems like this project needs a .clang-format file
include/stl2/view/const.hpp
Outdated
| const_view() = default; | ||
| constexpr explicit const_view(R base) : base_(std::move(base)) {} | ||
|
|
||
| constexpr auto base() const noexcept -> R { return base_; } |
There was a problem hiding this comment.
We usually don't use trailing return types.
include/stl2/view/const.hpp
Outdated
| constexpr __iterator<true> begin() const requires range<const R> | ||
| { | ||
| return __iterator<true>{*this, __stl2::begin(base_)}; | ||
| }; |
There was a problem hiding this comment.
Stray semicolon. Also, I think @CaseyCarter's style is to have the opening brace on the same line as the function's head.
include/stl2/view/const.hpp
Outdated
| return __stl2::size(base_); | ||
| } | ||
|
|
||
| constexpr auto size() const requires sized_range<R> |
There was a problem hiding this comment.
s/sized_range<R>/sized_range<const R>
| } | ||
| else | ||
| { | ||
| return __stl2::end(self.base_); |
There was a problem hiding this comment.
You'll need to make a custom sentinel type for this. @timsong-cpp pointed out that if a range adaptor has its own iterator, it needs its own sentinel so that we can compare both the adaptor's iterator and the adaptee's iterator against the sentinel.
include/stl2/view/const.hpp
Outdated
| }; | ||
|
|
||
| template <input_range R> | ||
| const_view(R&& r)->const_view<all_view<R>>; |
There was a problem hiding this comment.
Please also move this between the range adaptor and its iterator.
Co-Authored-By: Christopher Di Bella <cjdb.ns@gmail.com>
2 participants
I plan to make a proposal out of this and would like to have an implementation looked at beforehand.
Note that the adapter is named
views::as_constas opposed toconst_in range-v3. This is inspired by the fact thatviews::moveis conceptually applyingstd::moveon every element the same way that this view conceptually appliesstd::as_const.Another possible name is
const_elements, suggested by @cjdb.