Skip to content

[libc++][math] Provide overloads for cv-unqualified floating point types for std::isnormal #104773

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

Conversation

robincaloudis
Copy link
Contributor

@robincaloudis robincaloudis commented Aug 19, 2024

Why

Currently, the following does not work when compiled with clang:

#include <cmath>

struct ConvertibleToFloat {
    operator float();
};

bool test(ConvertibleToFloat x) {
    return std::isnormal(x);
}

See https://godbolt.org/z/5bos8v67T for differences with respect to msvc, gcc or icx. It fails for float, double and long double (all cv-unqualified floating-point types).

What

Test and provide overloads as expected by the ISO C++ standard. The classification/comparison function isnormal is defined since C++11 until C++23 as

bool isnormal( float num );
bool isnormal( double num );
bool isnormal( long double num );

and since C++23 as

constexpr bool isnormal( /* floating-point-type */ num );

for which "the library provides overloads for all cv-unqualified floating-point types as the type of the parameter num". See §28.7.1/1 in the ISO C++ standard or check cppreference.

This test shows that `isnormal()` does
not provide any overload for cv-unqualified
floating point type as actually defined
by the C++23 standard.

See §28.7.1/1.
@robincaloudis robincaloudis changed the title Provide overloads for cv-unqualified floating point types for isnormal [libc++][math] Provide overloads for cv-unqualified floating point types for isnormal Aug 19, 2024
@robincaloudis robincaloudis changed the title [libc++][math] Provide overloads for cv-unqualified floating point types for isnormal [libc++][math] Provide overloads for cv-unqualified floating point types for std::isnormal Aug 19, 2024
@robincaloudis robincaloudis marked this pull request as ready for review August 22, 2024 20:16
@robincaloudis robincaloudis requested a review from a team as a code owner August 22, 2024 20:16
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 22, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 22, 2024

@llvm/pr-subscribers-libcxx

Author: Robin Caloudis (robincaloudis)

Changes

Why

Currently, the following does not work when compiled with clang:

#include &lt;cmath&gt;

struct ConvertibleToFloat {
    operator float();
};

bool test(ConvertibleToFloat x) {
    return std::isnormal(x);
}

See https://godbolt.org/z/5bos8v67T for differences with respect to msvc, gcc or icx. If fails for double and long double as well. As the classification/comparison function isnormal is defined since C++11 until C++23 as

bool isnormal( float num );
bool isnormal( double num );
bool isnormal( long double num );

and since C++23 as

constexpr bool isnormal( /* floating-point-type */ num );

for which "the library provides overloads for all cv-unqualified floating-point types as the type of the parameter num", it is actually expected that convertibles types work due to the present of the overloads for all cv-unqualified floating point types. See §28.7.1/1 in the ISO C++ standard or check cppreference.

What

Test and provide overloads as expected by the ISO C++ standard.


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

2 Files Affected:

  • (modified) libcxx/include/__math/traits.h (+12)
  • (modified) libcxx/test/std/numerics/c.math/isnormal.pass.cpp (+14)
diff --git a/libcxx/include/__math/traits.h b/libcxx/include/__math/traits.h
index 35c283cc9e21ce..9fe1de66a80985 100644
--- a/libcxx/include/__math/traits.h
+++ b/libcxx/include/__math/traits.h
@@ -137,6 +137,18 @@ _LIBCPP_NODISCARD _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI bool isnor
   return __x != 0;
 }
 
+_LIBCPP_NODISCARD inline _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI bool isnormal(float __x) _NOEXCEPT {
+  return __builtin_isnormal(__x);
+}
+
+_LIBCPP_NODISCARD inline _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI bool isnormal(double __x) _NOEXCEPT {
+  return __builtin_isnormal(__x);
+}
+
+_LIBCPP_NODISCARD inline _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI bool isnormal(long double __x) _NOEXCEPT {
+  return __builtin_isnormal(__x);
+}
+
 // isgreater
 
 template <class _A1, class _A2, __enable_if_t<is_arithmetic<_A1>::value && is_arithmetic<_A2>::value, int> = 0>
diff --git a/libcxx/test/std/numerics/c.math/isnormal.pass.cpp b/libcxx/test/std/numerics/c.math/isnormal.pass.cpp
index c3b8f31359f988..76c3d13520d996 100644
--- a/libcxx/test/std/numerics/c.math/isnormal.pass.cpp
+++ b/libcxx/test/std/numerics/c.math/isnormal.pass.cpp
@@ -62,9 +62,23 @@ struct TestInt {
   }
 };
 
+template <typename T>
+struct ConvertibleTo {
+  operator T() const { return T(1); }
+};
+
 int main(int, char**) {
   types::for_each(types::floating_point_types(), TestFloat());
   types::for_each(types::integral_types(), TestInt());
 
+  // Make sure we can call `std::isnormal` with convertible types. This checks
+  // whether overloads for all cv-unqualified floating-point types are working
+  // as expected.
+  {
+    assert(std::isnormal(ConvertibleTo<float>()));
+    assert(std::isnormal(ConvertibleTo<double>()));
+    assert(std::isnormal(ConvertibleTo<long double>()));
+  }
+
   return 0;
 }

@@ -137,6 +137,18 @@ _LIBCPP_NODISCARD _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI bool isnor
return __x != 0;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The is_floating_point constrained overload is now unused, right? In that case it should be removed.

Copy link
Contributor Author

@robincaloudis robincaloudis Aug 28, 2024

Choose a reason for hiding this comment

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

Absolutely. You are right. Corrected it. In addition, I opened #106224 that removes the constrained overloads for std::{isinf,isfinite,isnan} too. Feel free to review both.

@robincaloudis robincaloudis force-pushed the rc-isnormal-overload-cv-unqualified-fp branch from 5a5e1cb to 33567a2 Compare August 27, 2024 13:35
Not needed anymore. Therefore, it can
safely be removed.
@philnik777 philnik777 merged commit 866bec7 into llvm:main Aug 28, 2024
56 checks passed
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.

3 participants