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.
Minor comments:
+#if __glibcxx_default_template_type_for_algorithm_values // C++ >= 26
+ template<indirectly_readable _Iter,
+ indirectly_regular_unary_invocable<_Iter> _Proj>
+ using projected_value_t = remove_cvref_t<invoke_result_t<_Proj&,
iter_value_t<_Iter>&>>;
+#endif
Please add a line break before the '=' to keep this line shorter.
+#if __glibcxx_default_template_type_for_algorithm_values
+ template<input_iterator _Iter, sentinel_for<_Iter> _Sent,
+ typename _Proj = identity, typename _Tp =
projected_value_t<_Iter, _Proj>>
+#else
And a line break after 'identity,' here please.
The new header has incorrect copyright info:
+// Copyright (C) 2007-2024 Free Software Foundation, Inc.
The dates are wrong, and you're contributing it under the DCO so
please use this form (with no dates):
// Copyright The GNU Toolchain Authors.
This is what I've been doing for all new files I add over the last few years.
The GPL and warranty text should remain, just change the copyright line.
**BUT** I don't think we should add a new header just for that anyway.
Hitting the filesystem to open a new header has a cost, and it's just
defining one macro anyway.
Could the macro just go in <bits/stl_iterator.h> ? Or stl_iterator_base_types?
Normally I'd say to put that kind of widely-used macro in
<bits/c++config.h> but that doesn't work here because it depends on
the feature test macro in <bits/version.h> which is included later.
