On Thu, 6 Feb 2020, Jonathan Wakely wrote:
> On 03/02/20 21:07 -0500, Patrick Palka wrote:
> > +#ifndef _RANGES_ALGO_H
> > +#define _RANGES_ALGO_H 1
> > +
> > +#if __cplusplus > 201703L
> > +
> > +#include <compare>
> > +#include <cmath>
> > +#include <iterator>
> > +// #include <bits/range_concepts.h>
>
> This line could be removed, or leave it as a reminder to me to
> refactor <ranges> so that the small utility pieces are in a small
> utility header (like <bits/ranges_concepts.h> that can be included
> instead of the whole of <ranges>.
I guess I'll leave it in then.
>
> > +#include <ranges>
> > +#include <bits/invoke.h>
> > +#include <bits/cpp_type_traits.h> // __is_byte
> > +#include <bits/random.h> // concept uniform_random_bit_generator
>
> I wonder if we want to move that concept to <bits/uniform_rand_dist.h>
> instead, which already exists to allow <algorithm> to avoid including
> the whole of <random>. If we do that, it would make sense to rename
> <bits/uniform_rand_dist.h> to <bits/random_fwd.h> or something like
> that.
That makes sense -- I can try to do that in a followup patch.
>
> > +
> > +#if __cpp_lib_concepts
> > +namespace std _GLIBCXX_VISIBILITY(default)
> > +{
> > +_GLIBCXX_BEGIN_NAMESPACE_VERSION
> > +namespace ranges
> > +{
> > + namespace __detail
> > + {
> > + template<typename _Tp>
> > + constexpr inline bool __is_normal_iterator = false;
>
> All these templates in the __detail namespace should be indented by
> two spaces after the template-head i.e.
>
> template<typename _Tp>
> constexpr inline bool __is_normal_iterator = false;
>
> (That indentation scheme has been in the libstdc++ style guide for
> longer than I've been contributing to the project, but it doesn't seem
> very popular with new contributors, and it wastes a level of
> indentation for templates, which means most of the library. Maybe we
> should revisit that convention.)
Fixed
>
>
> > + template<typename _Iter, typename _Out>
> > + using unary_transform_result = copy_result<_Iter, _Out>;
> > +
> > + template<input_iterator _Iter, sentinel_for<_Iter> _Sent,
> > + weakly_incrementable _Out,
> > + copy_constructible _Fp, typename _Proj = identity>
> > + requires writable<_Out, indirect_result_t<_Fp&, projected<_Iter,
> > _Proj>>>
>
> I have a pending patch to implement P1878R1, which renames writable
> (and a few other concepts). I'll wait until your patch is in, and
> change these places using it.
Sounds good.
>
> > + partial_sort_copy(_Iter1 __first, _Sent1 __last,
> > + _Iter2 __result_first, _Sent2 __result_last,
> > + _Comp __comp = {},
> > + _Proj1 __proj1 = {}, _Proj2 __proj2 = {})
> > + {
> > + if (__result_first == __result_last)
> > + {
> > + // TODO: Eliminating the variable __lasti triggers an ICE.
> > + auto __lasti = ranges::next(std::move(__first),
> > + std::move(__last));
> > + return {std::move(__lasti), std::move(__result_first)};
>
> Please try to reduce that and report it to bugzilla at some point,
> thanks.
Will do! Interestingly, it was an ICE in the middle-end. I wasn't able
to reproduce it anymore, but I'll try more carefully tomorrow.
>
> > +++ b/libstdc++-v3/testsuite/25_algorithms/all_of/constrained.cc
> > @@ -0,0 +1,90 @@
> > +// Copyright (C) 2019 Free Software Foundation, Inc.
>
> This should be 2020. That's the only change necessary though, please
> adjust that and commit to master. Great work, thank you!
Fixed. Thank you for the review! Patch committed, hopefully without
any fallout.
