On Fri, 13 Sept 2024 at 18:14, Patrick Palka <ppa...@redhat.com> wrote: > > On Fri, 13 Sep 2024, Jonathan Wakely wrote: > > > On Sat, 3 Aug 2024 at 00:10, Jonathan Wakely <jwak...@redhat.com> wrote: > > > > > > On Fri, 2 Aug 2024 at 23:49, Giuseppe D'Angelo > > > <giuseppe.dang...@kdab.com> wrote: > > > > > > > > Hello, > > > > > > > > as usual thank you for the review. V2 is attached. > > > > > > > > On 02/08/2024 14:38, Jonathan Wakely wrote: > > > > > On Fri, 2 Aug 2024 at 13:17, Jonathan Wakely <jwak...@redhat.com> > > > > > wrote: > > > > >> > > > > >> On Fri, 2 Aug 2024 at 11:45, Giuseppe D'Angelo wrote: > > > > >>> > > > > >>> Hello, > > > > >>> > > > > >>> The attached patch adds support for P2248R8 + P3217R0 (Enabling > > > > >>> list-initialization for algorithms, C++26). The big question is > > > > >>> whether > > > > >>> this keeps the code readable enough without introducing too much > > > > >>> #ifdef-ery, so any feedback is appreciated. > > > > >> > > > > >> Putting the extra args on the algorithmfwd.h declarations is a nice > > > > >> way to avoid any clutter on the definitions. I think that is very > > > > >> readable. > > > > >> Another option would be to not touch those forward declarations, but > > > > >> add new ones with the defaults: > > > > >> > > > > >> #if __glibcxx_default_template_type_for_algorithm_values > > > > >> // new declarations with default template args ... > > > > >> #endif > > > > >> > > > > >> But I think what you've done is good. > > > > > > > > I'll keep it then :) > > > > > > > > >> For ranges_algo.h I'm almost tempted to say we should just treat this > > > > >> as a DR, to avoid the #ifdef-ery. Almost. > > > > >> Is there any reason we can't rearrange the template parameters fo > > > > >> C++20 and C++23 mode? I don't think users are allowed to use explicit > > > > >> template argument lists for invoke e.g. ranges::find.operator()<Iter, > > > > >> Proj> so it should be unobservable if we change the order for C++20 > > > > >> (and just don't add the default args until C++26). That might reduce > > > > >> the differences to just a line or two for each CPO. > > > > > > > > Indeed, users cannot rely on any specific order of the template > > > > arguments when calling algorithms. This is > > > > > > > > https://eel.is/c++draft/algorithms.requirements#15 > > > > > > > > which has this note: > > > > > > > > "Consequently, an implementation can declare an algorithm with different > > > > template parameters than those presented" > > > > > > > > which of course does apply here: it's why P2248 could do these changes > > > > to begin with. The only reason why I kept them in the original order was > > > > a matter of caution, but sure, in the new patch I've changed them > > > > unconditionally and just used a macro to hide the default in pre-C++26 > > > > modes. This should keep the code clean(er). > > > > > > > > > > > > > The merged wording also removes the redundant 'typename' from the > > > > > default arguments, but I think we might still need that for Clang > > > > > compat. I'm not sure when Clang fully implemented "down with > > > > > typename", but it was still causing issues some time last year. > > > > > > > > I hope it's fine if I keep it. > > > > > > Yes, certainly. > > > > > > This looks good to me now, thanks. > > > > > > Reviewed-by: Jonathan Wakely <jwak...@redhat.com> > > > > > > Patrick, could you please review + test this some time next week? If > > > it's all fine and you're happy with it too, please push to trunk. > > Sorry for completely missing this!
No problem, I think we've both had a vacation that got in the way of this. > > > > +#if __glibcxx_algorithm_default_value_type // C++ >= 26 > > +# define _GLIBCXX26_ALGORITHM_DEFAULT_VALUE_TYPE(_Iterator) \ > > + = typename iterator_traits<_Iterator>::value_type > > +#else > > +# define _GLIBCXX26_ALGORITHM_DEFAULT_VALUE_TYPE(_It) > > Small nit, but these should consistently use _Iterator or _It as the > parameter name in both branches. Good point. N.B. we don't actually need to uglify macro arguments, but it doesn't hurt either. > Maybe we should abbreviate these macro names to _GLIBCXX26_ALGO_DEF_VAL_T etc > so that we can fit most uses on the same line as the template parameter > declaration (without exceeding line length limits)? Either way this patch > + your followup LGTM. Yes, I think that is preferable. And I realised that my new _GLIBCXX26_DEFAULT_VALUE_TYPE macro for the std::erase overloads is identical to one of the macros Guiseppe defined in bits/ranges_base.h so I combined those two, which freed up the shorter name for the constrained algos to be used for the 2-arg macro taking a projection. Updated patch attached.
