Skip to content

Commit 2406336

Browse files
bigerlbigerl
committed
address review
1 parent 86be9b3 commit 2406336

2 files changed

Lines changed: 58 additions & 51 deletions

File tree

‎include/dice/template-library/ranges.hpp‎

Lines changed: 51 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -8,33 +8,35 @@
88
#include <unordered_set>
99

1010
// all_of, any_of, none_of terminal algorithm
11-
#define DTL_DEFINE_PIPEABLE_RANGE_ALGO(ALGO_NAME) \
11+
#define DTL_DEFINE_PIPEABLE_RANGE_ALGO(algo_name) \
1212
namespace ranges_algo_detail { \
1313
template<typename Pred> \
14-
struct ALGO_NAME##_fn { \
14+
struct algo_name##_fn { \
15+
private: \
1516
Pred pred; \
1617
\
18+
public: \
1719
template<typename P> \
18-
constexpr explicit ALGO_NAME##_fn(P &&p) : pred(std::forward<P>(p)) {} \
20+
constexpr explicit algo_name##_fn(P &&p) : pred(std::forward<P>(p)) {} \
1921
\
2022
/* input_range causes no dangling risk since this adaptor is terminal */ \
2123
template<std::ranges::input_range R> \
2224
constexpr bool \
2325
operator()(R &&r) const { \
24-
return std::ranges::ALGO_NAME(r, pred); \
26+
return std::ranges::algo_name(r, pred); \
2527
} \
2628
\
2729
template<typename R> \
28-
friend auto operator|(R &&r, const ALGO_NAME##_fn &self) \
30+
friend auto operator|(R &&r, algo_name##_fn const &self) \
2931
-> decltype(self(std::forward<R>(r))) { \
3032
return self(std::forward<R>(r)); \
3133
} \
3234
}; \
3335
} \
3436
\
3537
template<typename Pred> \
36-
constexpr auto ALGO_NAME(Pred &&pred) { \
37-
return ranges_algo_detail::ALGO_NAME##_fn<std::decay_t<Pred>>( \
38+
constexpr auto algo_name(Pred &&pred) { \
39+
return ranges_algo_detail::algo_name##_fn<std::remove_cvref_t<Pred>>( \
3840
std::forward<Pred>(pred)); \
3941
}
4042

@@ -48,19 +50,15 @@ namespace dice::template_library {
4850
namespace dice::template_library {
4951
namespace ranges_algo_detail {
5052
struct empty_fn {
53+
5154
template<std::ranges::input_range R>
5255
constexpr bool operator()(R &&range) const {
53-
// Use std::ranges::empty if the expression is valid for the range type.
54-
if constexpr (requires { std::ranges::empty(range); }) {
55-
return std::ranges::empty(range);
56-
} else {
57-
// std::ranges::begin/end work for const and non-const ranges.
58-
return std::ranges::begin(range) == std::ranges::end(range);
59-
}
56+
static_assert(requires { std::ranges::empty(range); }, "std::ranges::empty requires an input range as it might otherwise be expensive to materialize the first element.");
57+
return std::ranges::empty(range);
6058
}
6159

6260
template<std::ranges::input_range R>
63-
friend constexpr auto operator|(R &&range, const empty_fn &self)
61+
friend constexpr auto operator|(R &&range, empty_fn const &self)
6462
-> decltype(self(std::forward<R>(range))) {
6563
return self(std::forward<R>(range));
6664
}
@@ -69,12 +67,11 @@ namespace dice::template_library {
6967
struct non_empty_fn {
7068
template<std::ranges::input_range R>
7169
constexpr bool operator()(R &&range) const {
72-
// Implemented as the negation of empty_fn for code reuse.
7370
return !empty_fn{}(std::forward<R>(range));
7471
}
7572

7673
template<std::ranges::input_range R>
77-
friend constexpr auto operator|(R &&range, const non_empty_fn &self)
74+
friend constexpr auto operator|(R &&range, non_empty_fn const &self)
7875
-> decltype(self(std::forward<R>(range))) {
7976
return self(std::forward<R>(range));
8077
}
@@ -107,22 +104,24 @@ namespace dice::template_library {
107104
namespace ranges_algo_detail {
108105
template<typename T, typename Pred>
109106
struct remove_element_fn {
107+
private:
110108
T value_;
111109
Pred pred_;
112110

111+
public:
113112
template<typename V, typename P>
114113
constexpr remove_element_fn(V &&value, P &&pred)
115114
: value_(std::forward<V>(value)), pred_(std::forward<P>(pred)) {}
116115

117116
template<std::ranges::viewable_range R>// viewable_range ensures there are no views into dangling stuff
118117
constexpr auto operator()(R &&range) const {
119-
return std::forward<R>(range) | std::views::filter([value = value_, pred = pred_](const auto &element) {
118+
return std::forward<R>(range) | std::views::filter([value = value_, pred = pred_](auto const &element) {
120119
return !std::invoke(pred, element, value);
121120
});
122121
}
123122

124123
template<std::ranges::viewable_range R>// viewable_range ensures there are no views into dangling stuff
125-
friend constexpr auto operator|(R &&range, const remove_element_fn &self)
124+
friend constexpr auto operator|(R &&range, remove_element_fn const &self)
126125
-> decltype(self(std::forward<R>(range))) {
127126
return self(std::forward<R>(range));
128127
}
@@ -138,7 +137,7 @@ namespace dice::template_library {
138137
*/
139138
template<typename T, typename Pred = std::ranges::equal_to>
140139
constexpr auto remove_element(T &&value, Pred &&pred = Pred{}) {
141-
return ranges_algo_detail::remove_element_fn<std::decay_t<T>, std::decay_t<Pred>>(
140+
return ranges_algo_detail::remove_element_fn<std::remove_cvref_t<T>, std::remove_cvref_t<Pred>>(
142141
std::forward<T>(value), std::forward<Pred>(pred));
143142
}
144143
}// namespace dice::template_library
@@ -157,23 +156,23 @@ namespace dice::template_library {
157156
template<std::ranges::input_range R>
158157
requires std::copyable<std::ranges::range_value_t<R>>
159158
constexpr bool operator()(R &&range) const {
160-
if (dice::template_library::empty(range)) {
159+
auto it = std::ranges::begin(range);
160+
if (it == std::ranges::end(range)) {
161161
return true;
162162
}
163-
164-
auto it = std::ranges::begin(range);
165-
const auto first_element = *(it++);
163+
const auto first_element = *it;
164+
++it;
166165

167166
// Check if all subsequent elements in the range match the first one.
168167
return std::ranges::subrange(it, std::ranges::end(range)) |
169-
dice::template_library::all_of([this, first_element](const auto &current_element) {
168+
dice::template_library::all_of([this, first_element](auto const &current_element) {
170169
return std::invoke(pred_, current_element, first_element);
171170
});
172171
}
173172

174173
template<std::ranges::input_range R>
175174
requires std::copyable<std::ranges::range_value_t<R>>
176-
friend constexpr auto operator|(R &&range, const all_equal_fn &self)
175+
friend constexpr auto operator|(R &&range, all_equal_fn const &self)
177176
-> decltype(self(std::forward<R>(range))) {
178177
return self(std::forward<R>(range));
179178
}
@@ -190,7 +189,7 @@ namespace dice::template_library {
190189
*/
191190
template<typename Pred = std::ranges::equal_to>
192191
constexpr auto all_equal(Pred &&pred = Pred{}) {
193-
return ranges_algo_detail::all_equal_fn<std::decay_t<Pred>>(
192+
return ranges_algo_detail::all_equal_fn<std::remove_cvref_t<Pred>>(
194193
std::forward<Pred>(pred));
195194
}
196195
}// namespace dice::template_library
@@ -205,12 +204,15 @@ namespace dice::template_library {
205204
private:
206205
struct iterator;
207206

208-
using ValueType = std::decay_t<std::ranges::range_value_t<V>>;
207+
using ValueType = std::remove_cvref_t<std::ranges::range_value_t<V>>;
209208
static_assert(std::copy_constructible<ValueType>, "The value type for unique must be copy-constructible.");
209+
static_assert((requires(const ValueType &v) { { std::hash<ValueType>{}(v) } -> std::convertible_to<std::size_t>; } && std::equality_comparable<V>) ||
210+
std::strict_weak_order<std::less<V>, V, V>,
211+
"The value type must be either hashable and equality comparable, or support less than..");
210212

211213
using SetType = std::conditional_t<
212214
// Check if std::hash is a valid expression for the value type.
213-
requires(const ValueType &v) { { std::hash<ValueType>{}(v) } -> std::convertible_to<std::size_t>; },
215+
requires(ValueType const &v) { { std::hash<ValueType>{}(v) } -> std::convertible_to<std::size_t>; },
214216
std::unordered_set<ValueType>,
215217
std::set<ValueType>>;
216218

@@ -284,7 +286,7 @@ namespace dice::template_library {
284286
}
285287

286288
template<std::ranges::viewable_range R>
287-
friend constexpr auto operator|(R &&r, const unique_fn &self)
289+
friend constexpr auto operator|(R &&r, unique_fn const &self)
288290
-> decltype(self(std::forward<R>(r))) {
289291
return self(std::forward<R>(r));
290292
}
@@ -310,9 +312,9 @@ namespace dice::template_library {
310312

311313
// The view that represents the generated sequence.
312314
template<std::integral T>
313-
struct range_generator_view : public std::ranges::view_interface<range_generator_view<T>> {
315+
struct range_generator_view : std::ranges::view_interface<range_generator_view<T>> {
314316
private:
315-
class iterator;// Forward-declare
317+
struct iterator;// Forward-declare
316318

317319
T start_{};
318320
T stop_{};
@@ -331,9 +333,9 @@ namespace dice::template_library {
331333

332334
// The iterator that generates the numbers on the fly.
333335
template<std::integral T>
334-
class range_generator_view<T>::iterator {
336+
struct range_generator_view<T>::iterator {
335337
private:
336-
const range_generator_view *parent_ = nullptr;
338+
range_generator_view const *parent_ = nullptr;
337339
T value_{};
338340

339341
public:
@@ -343,7 +345,7 @@ namespace dice::template_library {
343345
using difference_type = std::ptrdiff_t;
344346

345347
iterator() = default;
346-
constexpr explicit iterator(const range_generator_view *parent, T value)
348+
constexpr explicit iterator(range_generator_view const *parent, T value)
347349
: parent_(parent),
348350
value_(value) {}
349351

@@ -375,9 +377,9 @@ namespace dice::template_library {
375377
* - `range<T>(start, stop, step)`: Generates numbers from start, incrementing by step, until stop is met or passed.
376378
*/
377379
template<std::integral T, typename S = T>
378-
constexpr auto range(T arg1, T arg2, S step) {
380+
constexpr auto range(T start, T stop, S step) {
379381
static_assert(std::is_integral_v<S>, "Step must be an integral type.");
380-
return ranges_algo_detail::range_generator_view<T>(arg1, arg2, static_cast<std::make_signed_t<T>>(step));
382+
return ranges_algo_detail::range_generator_view<T>(start, stop, static_cast<std::make_signed_t<T>>(step));
381383
}
382384

383385
template<std::integral T>
@@ -397,25 +399,24 @@ namespace dice::template_library {
397399
namespace ranges_algo_detail {
398400
template<typename Pred>
399401
struct all_distinct_fn {
402+
private:
400403
Pred pred_;
401404

405+
public:
402406
template<typename P>
403407
constexpr explicit all_distinct_fn(P &&pred)
404408
: pred_(std::forward<P>(pred)) {}
405409

406410
template<std::ranges::input_range R>
407411
constexpr bool operator()(R &&range) const {
408-
using ValueType = std::decay_t<std::ranges::range_value_t<R>>;
412+
using ValueType = std::remove_cvref_t<std::ranges::range_value_t<R>>;
409413

410-
if (dice::template_library::empty(range)) {
411-
return true;
412-
}
413414

414415
// backend_selection
415416
constexpr bool is_default_pred = std::is_same_v<Pred, std::ranges::equal_to>;
416417

417418
constexpr bool hash_set_usable = std::copy_constructible<ValueType> &&
418-
requires(const ValueType &v) {
419+
requires(ValueType const &v) {
419420
{ std::hash<ValueType>{}(v) } -> std::convertible_to<std::size_t>;
420421
};
421422

@@ -424,21 +425,26 @@ namespace dice::template_library {
424425
is_default_pred;// non-default predicates cannot be used with the ordered set
425426
static_assert(hash_set_usable || ordered_set_usable, "The elements of the consumed range are neither compatible with std::unordered_set nor with std::set.");
426427

428+
auto it = std::ranges::begin(range);
429+
if (it == std::ranges::end(range)) {
430+
return true;
431+
}
432+
427433
auto seen = [&] {
428434
if constexpr (hash_set_usable) {
429435
return std::unordered_set<ValueType, std::hash<ValueType>, Pred>{1, {}, pred_};
430436
} else {
431437
return std::set<ValueType>{};
432438
}
433439
}();
434-
for (const auto &element : range) {
440+
for (const auto &element : std::ranges::subrange{it, std::ranges::end(range)}) {
435441
if (!seen.insert(element).second) { return false; }
436442
}
437443
return true;
438444
}
439445

440446
template<std::ranges::input_range R>
441-
friend constexpr auto operator|(R &&range, const all_distinct_fn &self)
447+
friend constexpr auto operator|(R &&range, all_distinct_fn const &self)
442448
-> decltype(self(std::forward<R>(range))) {
443449
return self(std::forward<R>(range));
444450
}
@@ -458,7 +464,7 @@ namespace dice::template_library {
458464
*/
459465
template<typename Pred = std::ranges::equal_to>
460466
constexpr auto all_distinct(Pred &&pred = Pred{}) {
461-
return ranges_algo_detail::all_distinct_fn<std::decay_t<Pred>>(
467+
return ranges_algo_detail::all_distinct_fn<std::remove_cvref_t<Pred>>(
462468
std::forward<Pred>(pred));
463469
}
464470

‎tests/tests_ranges.cpp‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,8 @@ TEST_SUITE("unique range adaptor") {
206206

207207
TEST_CASE("edge cases") {
208208
std::vector<int> empty_vec{};
209-
CHECK((empty_vec | dtl::unique | dtl::empty));
209+
auto result_empty_vec = empty_vec | dtl::unique;
210+
CHECK(std::begin(result_empty_vec) == std::end(result_empty_vec));
210211

211212
std::vector all_unique{1, 2, 3, 4, 5};
212213
auto result_all_unique = all_unique | dtl::unique;
@@ -268,7 +269,7 @@ TEST_SUITE("range view") {
268269

269270
// A range up to 0 should be empty
270271
auto view3 = dtl::range<int>(0);
271-
CHECK((view3 | dtl::empty));
272+
CHECK(std::ranges::begin(view3) == std::ranges::end(view3));
272273

273274
// A range up to 1 should contain only 0
274275
auto view4 = dtl::range<int>(1);
@@ -283,11 +284,11 @@ TEST_SUITE("range view") {
283284

284285
// Start is after stop, should be empty
285286
auto view2 = dtl::range<int>(5, 2);
286-
CHECK((view2 | dtl::empty));
287+
CHECK(std::begin(view2) == std::end(view2));
287288

288289
// Start is equal to stop, should be empty
289290
auto view3 = dtl::range<int>(5, 5);
290-
CHECK((view3 | dtl::empty));
291+
CHECK(std::begin(view3) == std::end(view3));
291292

292293
// negative numbers
293294
auto view4 = dtl::range<int>(-2, 2);
@@ -312,7 +313,7 @@ TEST_SUITE("range view") {
312313

313314
// wrong direction
314315
auto view_wrong_dir = dtl::range<int>(10, 0, 2);
315-
CHECK((view_wrong_dir | dtl::empty));
316+
CHECK(std::begin(view_wrong_dir) == std::end(view_wrong_dir));
316317
}
317318

318319
SUBCASE("negative step") {
@@ -326,7 +327,7 @@ TEST_SUITE("range view") {
326327

327328
// wrong direction
328329
auto view_wrong_dir = dtl::range<int>(0, 10, -2);
329-
CHECK((view_wrong_dir | dtl::empty));
330+
CHECK(std::begin(view_wrong_dir) == std::end(view_wrong_dir));
330331
}
331332
}
332333
}

0 commit comments

Comments
 (0)