Re: [PATCH 2/2] libstdc++: Replace all manual FTM definitions and use

[Jonathan Wakely <jwakely@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3123 bytes --]

On Sat, 29 Apr 2023 at 11:24, Arsen Arsenović via Libstdc++ <
libstdc++@gcc.gnu.org> wrote:

> libstdc++-v3/ChangeLog:
>
>         * libsupc++/typeinfo: Switch to bits/version.h for
>         __cpp_lib_constexpr_typeinfo.
>
>
Does this change have an impact on compilation speed?
With this change we'll be re-including bits/version.h multiple times in
most compilations, and unlike other headers the preprocessor can't optimize
away the second and subsequent times its' included, because the header
isn't idempotent.
It will only affect the preprocessing phase, which is a fraction of the
time taken by template instantiation and middle end optimizations, but I'd
like to know it's not *too* expensive before committing to this approach.



> @@ -234,9 +234,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        return __atomic_test_and_set (&_M_i, int(__m));
>      }
>
> -#if __cplusplus > 201703L
> -#define __cpp_lib_atomic_flag_test 201907L
> -
> +#ifdef __cpp_lib_atomic_flag_test
>      _GLIBCXX_ALWAYS_INLINE bool
>      test(memory_order __m = memory_order_seq_cst) const noexcept
>      {
>

This is more "structured" and maintainable than the current ad-hoc way we
deal with FTMs, but this seems like a readability/usability regression in
terms of being able to open the header and see "ah this feature is only
available for C++20 and up". Instead you can see it's available for the
specified FTM, but now you have to go and find where that's defined, and
that's not even defined in C++, it's in the version.def file. It's also
defined in bits/version.h, but that's a generated file and so is very
verbose and long.


diff --git a/libstdc++-v3/include/bits/move_only_function.h
> b/libstdc++-v3/include/bits/move_only_function.h
> index 71d52074978..81d7d9f7c0a 100644
> --- a/libstdc++-v3/include/bits/move_only_function.h
> +++ b/libstdc++-v3/include/bits/move_only_function.h
> @@ -32,7 +32,10 @@
>
>  #pragma GCC system_header
>
> -#if __cplusplus > 202002L
> +#define __glibcxx_want_move_only_function
> +#include <bits/version.h>
> +
> +#ifdef __cpp_lib_move_only_function
>

Here's another case where I think the __cplusplus > 202002L is more
discoverable.

Although maybe I'm biased, because I look at that and immediately see
"C++23 and up". Maybe the average user finds that less clear. Maybe the
average user doesn't need to look at this anyway, but I know *I* do it
fairly often.

I wonder if it would help if we kept a comment there with a (possibly
imprecise) hint about the conditions under which the feature is defined. So
in this case:

// Only defined for C++23
#ifdef __cpp_lib_move_only_function

That retains the info that's currently there, and is even more readable
than the __cplusplus check.

There's a risk that those comments would get out of step with reality,
which is one of the things this patch set aims to solve. But I think in
practice that's unlikely. std::move_only_function isn't suddenly going to
become available in C++20, or stop being available in C++23 and move to
C++26.

What do you think?