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

["Arsen Arsenović" <arsen@aarsen.me>]

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


Jonathan Wakely <jwakely@redhat.com> writes:

> 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?

I think that's reasonable.  And, yes, I doubt these conditions change
much.  I'll go over the conditions and insert a hint.
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]