Skip to content

[libc++][NFC] Simplify the implementation of __promote #81379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 8, 2024

Conversation

philnik777
Copy link
Contributor

This depends on enabling the use of extensions.

@philnik777 philnik777 requested a review from a team as a code owner February 10, 2024 21:08
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Feb 10, 2024
@llvmbot
Copy link
Member

llvmbot commented Feb 10, 2024

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

This depends on enabling the use of extensions.


Full diff: https://github.com/llvm/llvm-project/pull/81379.diff

1 Files Affected:

  • (modified) libcxx/include/__type_traits/promote.h (+6-53)
diff --git a/libcxx/include/__type_traits/promote.h b/libcxx/include/__type_traits/promote.h
index e22b4a422c2c80..65eb7b37403edf 100644
--- a/libcxx/include/__type_traits/promote.h
+++ b/libcxx/include/__type_traits/promote.h
@@ -11,8 +11,7 @@
 
 #include <__config>
 #include <__type_traits/integral_constant.h>
-#include <__type_traits/is_same.h>
-#include <__utility/declval.h>
+#include <__type_traits/is_arithmetic.h>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
@@ -20,9 +19,10 @@
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-template <class _Tp>
-struct __numeric_type {
-  static void __test(...);
+template <class... _Args>
+class __promote {
+  static_assert((is_arithmetic<_Args>::value && ...));
+
   static float __test(float);
   static double __test(char);
   static double __test(int);
@@ -38,57 +38,10 @@ struct __numeric_type {
   static double __test(double);
   static long double __test(long double);
 
-  typedef decltype(__test(std::declval<_Tp>())) type;
-  static const bool value = _IsNotSame<type, void>::value;
-};
-
-template <>
-struct __numeric_type<void> {
-  static const bool value = true;
-};
-
-template <class _A1,
-          class _A2 = void,
-          class _A3 = void,
-          bool      = __numeric_type<_A1>::value && __numeric_type<_A2>::value && __numeric_type<_A3>::value>
-class __promote_imp {
-public:
-  static const bool value = false;
-};
-
-template <class _A1, class _A2, class _A3>
-class __promote_imp<_A1, _A2, _A3, true> {
-private:
-  typedef typename __promote_imp<_A1>::type __type1;
-  typedef typename __promote_imp<_A2>::type __type2;
-  typedef typename __promote_imp<_A3>::type __type3;
-
 public:
-  typedef decltype(__type1() + __type2() + __type3()) type;
-  static const bool value = true;
+  using type = decltype((__test(_Args()) + ...));
 };
 
-template <class _A1, class _A2>
-class __promote_imp<_A1, _A2, void, true> {
-private:
-  typedef typename __promote_imp<_A1>::type __type1;
-  typedef typename __promote_imp<_A2>::type __type2;
-
-public:
-  typedef decltype(__type1() + __type2()) type;
-  static const bool value = true;
-};
-
-template <class _A1>
-class __promote_imp<_A1, void, void, true> {
-public:
-  typedef typename __numeric_type<_A1>::type type;
-  static const bool value = true;
-};
-
-template <class _A1, class _A2 = void, class _A3 = void>
-class __promote : public __promote_imp<_A1, _A2, _A3> {};
-
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___TYPE_TRAITS_PROMOTE_H

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should #if the old code as a whole for Clang 17 just to get this patch unblocked, and we can remove that "workaround" when we drop support for Clang 17. It's so easy to clean these up that I wouldn't stall this patch just for that reason.

@philnik777 philnik777 force-pushed the simplify_promote branch 2 times, most recently from 2f06008 to 5416e73 Compare June 7, 2024 05:57
@philnik777 philnik777 merged commit c8992fb into llvm:main Jun 8, 2024
53 checks passed
nekoshirro pushed a commit to nekoshirro/Alchemist-LLVM that referenced this pull request Jun 9, 2024
This depends on enabling the use of extensions.

Signed-off-by: Hafidz Muzakky <[email protected]>
@EricWF
Copy link
Member

EricWF commented Jun 10, 2024

I think we should #if the old code as a whole for Clang 17 just to get this patch unblocked, and we can remove that "workaround" when we drop support for Clang 17. It's so easy to clean these up that I wouldn't stall this patch just for that reason.

This is supposed to be a simplification, meaning it only provides value if it simplifies the code. We've just added a second implementation, which is not a simplification, it's a complication.

There's no need to land code like this while it doesn't provided the stated value.

This should have waited until all supported compilers allowed us to do it.

@HerrCai0907 HerrCai0907 mentioned this pull request Jun 13, 2024
@philnik777 philnik777 deleted the simplify_promote branch September 25, 2024 13:41
frederick-vs-ja added a commit that referenced this pull request Oct 29, 2024
…#110235)

Fixes #109858.

The changes in #81379 broke some 3rd party library code that expected
usability of `std::complex<NonFloatingPoint>`. Although such code isn't
portable per [complex.numbers.general]/2, it might be better to make
these additional overloads not to interfere overload resolution too
much.

---------

Co-authored-by: Louis Dionne <[email protected]>
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…llvm#110235)

Fixes llvm#109858.

The changes in llvm#81379 broke some 3rd party library code that expected
usability of `std::complex<NonFloatingPoint>`. Although such code isn't
portable per [complex.numbers.general]/2, it might be better to make
these additional overloads not to interfere overload resolution too
much.

---------

Co-authored-by: Louis Dionne <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants