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. > > 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. > > I think there's a misunderstanding about how this works: > +#define __glibcxx_want_default_template_type_for_algorithm_values > +#include <bits/version.h> > > For every standard __cpp_lib_foo feature test macro there is also an > internal __glibcxx_foo macro. The standard __cpp_lib_foo one should > *only* be defined in the headers that the standard documents to define > the macro (and in <version>, which happens automatically). That > happens when you define __glibcxx_want_foo before including > <bits/version.h>, so that must only be done in the standard headers > that should define the __cpp_lib_foo version. > > For this feature, the standard says: > #define __cpp_lib_default_template_type_for_algorithm_values YYYYMML // also > in > // <algorithm>, <ranges>, > // <string>, <deque>, <list>, <forward_list>, <vector> > So you should put the __glibcxx_want_xxx macro in the headers named > above, but directly in the standard <vector> header (and <string>, > etc.) not in <bits/stl_vector.h> or any other internal detail headers. > > It's wrong to use __glibcxx_want_foo in <bits/ranges_algo.h> and in > your new header. > The internal headers like <bits/ranges_algo.h> should use the > __glibcxx_foo macro, not the __cpp_lib_foo one. The __glibcxx_foo one > is defined without the "want" macro, so is always defined when the > feature is supported. That means you can test the __glibcxx_foo macro > after including <bits/version.h> *without* saying you "want" the > standard __cpp_lib_foo macro. > > I don't think that stuff is documented yet, so I'll put something like > the text above into the manual.
Oh also, the editor renamed the feature test macro to __cpp_lib_algorithm_default_value_type so it fits on the page ;-) Your patch should use that, not the one in the proposal. 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.
