[libc++] Use remove_pointer builtin as library traits

[Chilledheart]

In new GCC release (such as 14.1), a new warning is added that all traits are not to be used directly but library traits.

Keep the same structure with gcc's library traits to prevent future issues.

see https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/type_traits#L2105 and https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/type_traits#L2146.

Fixes #91831.

[github-actions[bot]]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Keeyou (Chilledheart)

Changes

Gcc's remove_pointer builtin is added in 14.1 release but the implementation is incomplete. see https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/type_traits#L2105 and https://github.com/gcc-mirror/gcc/commit/40dd7a5fe5d. Fixes #91831. --- Full diff: https://github.com/llvm/llvm-project/pull/91838.diff 1 Files Affected: - (modified) libcxx/include/__type_traits/remove_pointer.h (+1-1) ``````````diff diff --git a/libcxx/include/__type_traits/remove_pointer.h b/libcxx/include/__type_traits/remove_pointer.h index 54390a1939f7d..9b74cd723c7e8 100644 --- a/libcxx/include/__type_traits/remove_pointer.h +++ b/libcxx/include/__type_traits/remove_pointer.h @@ -24,7 +24,7 @@ struct remove_pointer { }; template -using __remove_pointer_t = __remove_pointer(_Tp); +using __remove_pointer_t = remove_pointer<_Tp>::type; #else // clang-format off template struct _LIBCPP_TEMPLATE_VIS remove_pointer {typedef _LIBCPP_NODEBUG _Tp type;}; ``````````

[Chilledheart]

libc++ is well-tested with clang for both manual tests and CI. So it is sufficient to exclude other compilers to use their remove_pointer builtin @frederick-vs-ja .

[Chilledheart]

As described in https://github.com/llvm/llvm-project/issues/91831, from GCC's aspect the warning tells us its design decision:

error: use of built-in trait \u2018__remove_pointer(typename std::__Cr::decay<_Tp>::type)\u2019 in function signature; use library traits instead

That is that "all traits are not to be used directly" but the "library traits". Below is the gcc's library traits (libstdc++).

  template<typename _Tp>
    struct remove_pointer
    { using type = __remove_pointer(_Tp); };

and

  /// Alias template for remove_pointer
  template<typename _Tp>
    using remove_pointer_t = typename remove_pointer<_Tp>::type;

To fix this issue (along with other new builts for library traits in new gcc releases), the right way is use "change the type alias to use the alias from the struct always" as I talked with a gcc maintainer.

For example, gcc's add_pointer_t traits has the same structure with remove_pointer_t. If we face the same issue in the future. We can handle it with the same way.

  /// Alias template for add_pointer
  template<typename _Tp>
    using add_pointer_t = typename add_pointer<_Tp>::type;

[philnik777]

I agree that it doesn't make a ton of sense to guard this on the version, since the GCC folks make it very clear that they don't intend to support the use of the builtin directly. But I also don't think that we should pessimize the Clang version here. We should simply resolve the __remove_pointer_t alias to either remove_pointer<>::type for GCC or __remove_pointer() for Clang.

[Chilledheart]

I agree that it doesn't make a ton of sense to guard this on the version, since the GCC folks make it very clear that they don't intend to support the use of the builtin directly.

For type traits, no. It is because type traits structure doesn't require demangle names. For value traits, yes. They are using the builtin directly to avoid intermediate symbols.

We have the same issues with __add_pointer as well where there're lacking structure used for type alias.

[Chilledheart]

We should simply resolve the __remove_pointer_t alias to either remove_pointer<>::type for GCC or __remove_pointer() for Clang.

It's up to you. You're the maintainer. But that reason doesn't convince me because it makes things complex especially the patch. As a user of libc++, I want things go upstream because it makes maintainment easier without introducing some complex code.

[mordante]

I agree that it doesn't make a ton of sense to guard this on the version, since the GCC folks make it very clear that they don't intend to support the use of the builtin directly.

FYI My original remark was based on the commit message Gcc's remove_pointer builtin is added in 14.1 release but the implementation is incomplete. That did not make it clear GCC doesn't intend to support the builtin directly.

[philnik777]

I agree that it doesn't make a ton of sense to guard this on the version, since the GCC folks make it very clear that they don't intend to support the use of the builtin directly.

For type traits, no. It is because type traits structure doesn't require demangle names. For value traits, yes. They are using the builtin directly to avoid intermediate symbols.

We have the same issues with __add_pointer as well where there're lacking structure used for type alias.

GCC doesn't mangle any of the builtin type traits, so you can't use __is_whatever directly either. You have to go through either a class template or a variable template. Both have their own mangling, while an alias template doesn't.

We should simply resolve the __remove_pointer_t alias to either remove_pointer<>::type for GCC or __remove_pointer() for Clang.

It's up to you. You're the maintainer. But that reason doesn't convince me because it makes things complex especially the patch. As a user of libc++, I want things go upstream because it makes maintainment easier without introducing some complex code.

I'm not sure why you think this makes the patch complex. That's like four lines of code.

[Chilledheart]

Member

For type traits, no. There is no builtin function for type traits directly.

[Chilledheart]

I agree that it doesn't make a ton of sense to guard this on the version, since the GCC folks make it very clear that they don't intend to support the use of the builtin directly.

For type traits, no. It is because type traits structure doesn't require demangle names. For value traits, yes. They are using the builtin directly to avoid intermediate symbols. We have the same issues with __add_pointer as well where there're lacking structure used for type alias.

GCC doesn't mangle any of the builtin type traits, so you can't use __is_whatever directly either. You have to go through either a class template or a variable template. Both have their own mangling, while an alias template doesn't.

We should simply resolve the __remove_pointer_t alias to either remove_pointer<>::type for GCC or __remove_pointer() for Clang.

It's up to you. You're the maintainer. But that reason doesn't convince me because it makes things complex especially the patch. As a user of libc++, I want things go upstream because it makes maintainment easier without introducing some complex code.

I'm not sure why you think this makes the patch complex. That's like four lines of code.

I don't like that patch. If you want to make it, make it.

Finally, you will have to add 4x14 lines code at least, see https://github.com/llvm/llvm-project/issues/91831#issuecomment-2105592054. For other type traits, it can be even more complex.

[Chilledheart]

@philnik777 I contributed to llvm project before such as d431932d3895eaff189cba429289671ff68b26eb and 0c6fed5716c75285181db8a20e5f84ddec11f76a when llvm still sat in Phabricator. But I don't agree with your way of handling code. If libc++ has different standards with llvm, it makes sense.

[philnik777]

I don't like that patch. If you want to make it, make it.

Finally, you will have to add 4x14 lines code at least, see #91831 (comment). For other type traits, it can be even more complex.

You're not required to like it, but just saying you don't like it doesn't tell me at all why. 4 lines per type trait really doesn't seem that bad. Also note that there only have to be changes for the transformation type traits, not the value type traits. I'm also not going to write a patch, since I don't have access to GCC 14 yet. You submitted a patch to make changes to libc++. If you don't want to, don't do it. Nobody makes you.

@philnik777 I contributed to llvm project before such as d431932 and 0c6fed5 when llvm still sat in Phabricator. But I don't agree with your way of handling code. If libc++ has different standards with llvm, it makes sense.

You're free to disagree, but please make a point I can respond to instead of just saying that you disagree. Please tell me why. Any yes, libc++ is different from other parts of the project. libc++ is a standard library and not a compiler after all.

[Chilledheart]

@philnik777 You can make it in the way you like it. But I don't like the bad code style you stands for. Hopefully, you will make 14 patches to make things working again.

[Chilledheart]

I am a C++ coder so the only way you can convince me is in c++ way. But sadly, you don't. You prove you and your standard libraries sit at a bad code style.