[libc++] Fix acceptance of convertible-to-{float,double,long double} in std::isfinite() #98841

robincaloudis · 2024-07-14T22:13:29Z

The following error is raised if convertibles are used as arguments to isfinite(): "error: "no matching function for call to 'isfinite'".

This time, we test for the overload long double.

llvmbot · 2024-07-14T22:14:02Z

@llvm/pr-subscribers-libcxx

Author: Robin Caloudis (robincaloudis)

Changes

Closes #98816.

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

2 Files Affected:

(modified) libcxx/include/__math/traits.h (+14)
(modified) libcxx/test/std/numerics/c.math/isfinite.pass.cpp (+33)

diff --git a/libcxx/include/__math/traits.h b/libcxx/include/__math/traits.h
index a448266797557..07e5a74ac2362 100644
--- a/libcxx/include/__math/traits.h
+++ b/libcxx/include/__math/traits.h
@@ -55,6 +55,20 @@ _LIBCPP_NODISCARD _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI bool isfin
   return true;
 }
 
+#ifdef _LIBCPP_PREFERRED_OVERLOAD
+_LIBCPP_NODISCARD inline _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI bool isfinite(float __x) _NOEXCEPT {
+  return __builtin_isfinite(__x);
+}
+
+_LIBCPP_NODISCARD inline _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI _LIBCPP_PREFERRED_OVERLOAD bool
+isfinite(double __x) _NOEXCEPT {
+  return __builtin_isfinite(__x);
+}
+
+_LIBCPP_NODISCARD inline _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI bool isfinite(long double __x) _NOEXCEPT {
+  return __builtin_isfinite(__x);
+}
+#endif
 // isinf
 
 template <class _A1, __enable_if_t<is_arithmetic<_A1>::value && numeric_limits<_A1>::has_infinity, int> = 0>
diff --git a/libcxx/test/std/numerics/c.math/isfinite.pass.cpp b/libcxx/test/std/numerics/c.math/isfinite.pass.cpp
index 6bbc3aaac6d13..56d9550348a9b 100644
--- a/libcxx/test/std/numerics/c.math/isfinite.pass.cpp
+++ b/libcxx/test/std/numerics/c.math/isfinite.pass.cpp
@@ -62,9 +62,42 @@ struct TestInt {
   }
 };
 
+struct ConvertibleFloat {
+    int value;
+
+    ConvertibleFloat(int v) : value(v) {}
+
+    operator float() const {
+        return static_cast<float>(value);
+    }
+};
+
+struct ConvertibleDouble {
+    int value;
+
+    ConvertibleDouble(int v) : value(v) {}
+
+    operator double() const {
+        return static_cast<double>(value);
+    }
+};
+
+struct ConvertibleLongDouble {
+    int value;
+
+    ConvertibleLongDouble(int v) : value(v) {}
+
+    operator long double() const {
+        return static_cast<long double>(value);
+    }
+};
+
 int main(int, char**) {
   types::for_each(types::floating_point_types(), TestFloat());
   types::for_each(types::integral_types(), TestInt());
+  assert(std::isfinite(ConvertibleFloat(0)));
+  assert(std::isfinite(ConvertibleDouble(0)));
+  assert(std::isfinite(ConvertibleLongDouble(0)));
 
   return 0;
 }

github-actions · 2024-07-14T22:17:09Z

✅ With the latest revision this PR passed the C/C++ code formatter.

AngryLoki · 2024-07-15T09:18:45Z

Fails in gcc test... Looks like std::isnan _LIBCPP_PREFERRED_OVERLOAD also never worked in gcc too and it was not covered with tests. I guess it is possible to implement it in gcc (example: https://godbolt.org/z/vcrGWzcvj). But I don't know if this level of complication is needed. libstdc++ has no such thing as "preferred overload" and just fails with call to 'isfinite' is ambiguous when there are multiple user-defined conversion functions.

ldionne · 2024-07-15T18:59:05Z

Thanks for the patch! Is there a reason why we must apply _LIBCPP_PREFERRED_OVERLOAD at all? This might be a naive question, but GCC implements the same thing without any trickery so I wonder why we need _LIBCPP_PREFERRED_OVERLOAD in the first place.

robincaloudis · 2024-07-16T14:17:36Z

Thanks for the hint! You two are completely right. _LIBCPP_PREFERRED_OVERLOAD is not needed. So far, all tests look good.

Once this is merged, I will also properly test the convertible types for std::{isnan, isinf}in #98952 as they seem to be broken due to the usage of _LIBCPP_PREFERRED_OVERLOAD.

philnik777

Could you also check the other traits to make sure they're correctly implemented?

libcxx/test/std/numerics/c.math/isfinite.pass.cpp

robincaloudis · 2024-07-16T14:21:49Z

Could you also check the other traits to make sure they're correctly implemented?

Sure, I'll check them.

ldionne

LGTM once CI is done running. MacOS is a bit backed up right now due to an outage but it should get there eventually. Thanks!

libcxx/test/std/numerics/c.math/isfinite.pass.cpp

ldionne · 2024-07-16T14:26:39Z

Sorry, @philnik777 and I got in a race condition. Please make sure to satisfy Nikolas' requests as well. Thanks :-)

ldionne

Can be merged once CI is done running.

robincaloudis · 2024-07-18T10:57:00Z

CI is done running. Could you merge this PR? I lack write access. Thanks!

AngryLoki · 2024-07-19T07:31:17Z

That was quick! Thanks everyone!

…in std::isfinite() (llvm#98841) Closes llvm#98816.

…in std::isfinite() (#98841) Summary: Closes #98816. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250840

…and `std::isinf()` (#98952) Following up on #98841. Changes: - Properly test convertible types for `std::isnan()` and `std::inf()` - Tighten conditional in `cmath.pass.cpp` (Find insights on `_LIBCPP_PREFERRED_OVERLOAD` below) - Tighten preprocessor guard in `traits.h` Insights into why `_LIBCPP_PREFERRED_OVERLOAD` is needed: (i) When libc++ is layered on top of glibc on Linux, glibc's `math.h` is included. When compiling with `-std=c++03`, this header brings the function declaration of `isinf(double)` [1] and `isnan(double)` [2] into scope. This differs from the C99 Standard as only the macros `#define isnan(arg)` and `#define isinf(arg)` are expected. Therefore, libc++ needs to respect the presense of the `double` overload and cannot redefine it as it will conflict with the declaration already in scope. For `-std=c++11` and beyond this issue is fixed, as glibc guards both the `isinf` and `isnan` by preprocessor macros. (ii) When libc++ is layered on top of Bionic's libc, `math.h` exposes a function prototype for `isinf(double)` with return type `int`. This function prototype in Bionic's libc is not guarded by any preprocessor macros [3]. `_LIBCPP_PREFERRED_OVERLOAD` specifies that a given overload is a better match than an otherwise equally good function declaration. This is implemented in modern versions of Clang via `__attribute__((__enable_if__))`, and not elsewhere. See [4] for details. We use `_LIBCPP_PREFERRED_OVERLOAD` to define overloads in the global namespace that displace the overloads provided by the C libraries mentioned above. [1]: https://github.com/bminor/glibc/blob/fe9408087583fd7a6f61bb0dbcf2fd4e83186afa/math/bits/mathcalls.h#L185-L194 [2]: https://github.com/bminor/glibc/blob/fe9408087583fd7a6f61bb0dbcf2fd4e83186afa/math/bits/mathcalls.h#L222-L231 [3]: https://cs.android.com/android/platform/superproject/+/master:bionic/libc/include/math.h;l=322-323;drc=master?hl=fr-BE%22https:%2F%2Fsupport.google.com%2Fmerchants%2Fanswer%2F188494%5C%22%22https:%2F%2Fsupport.google.com%2Fmerchants%2Fanswer%2F188494%5C%22 [4]: 5fd17ab

…sfinite}` (#106224) ## Why Since #98841 and #98952, the constrained overloads are unused and not needed anymore as we added explicit overloads for all floating point types. I forgot to remove them in the mentioned PRs. ## What Remove them.

…sfinite}` (llvm#106224) ## Why Since llvm#98841 and llvm#98952, the constrained overloads are unused and not needed anymore as we added explicit overloads for all floating point types. I forgot to remove them in the mentioned PRs. ## What Remove them.

robincaloudis added 6 commits July 15, 2024 00:01

Reproduce error by a test

3827e6f

The following error is raised if convertibles are used as arguments to isfinite(): "error: "no matching function for call to 'isfinite'".

Provide float overload

9dcd0cf

Reproduce error by a test for double overload

121bad4

Provide double overload for isfinite()

23f4a4c

Reproduce another error by a test

90d2737

This time, we test for the overload long double.

Provide long double overload

9df4a55

robincaloudis requested a review from a team as a code owner July 14, 2024 22:13

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 14, 2024

robincaloudis marked this pull request as draft July 14, 2024 22:15

Fix formatting

86f5933

robincaloudis marked this pull request as ready for review July 14, 2024 22:37

robincaloudis force-pushed the rc-isfinite-overload branch 5 times, most recently from 8d9bd88 to 276d412 Compare July 15, 2024 22:07

Remove preprocessor directive

c858bb6

robincaloudis force-pushed the rc-isfinite-overload branch from 276d412 to c858bb6 Compare July 16, 2024 05:56

philnik777 reviewed Jul 16, 2024

View reviewed changes

libcxx/test/std/numerics/c.math/isfinite.pass.cpp Outdated Show resolved Hide resolved

ldionne approved these changes Jul 16, 2024

View reviewed changes

libcxx/test/std/numerics/c.math/isfinite.pass.cpp Outdated Show resolved Hide resolved

Extract template and leave comment

8d56623

robincaloudis force-pushed the rc-isfinite-overload branch from 2e6a04d to 8d56623 Compare July 16, 2024 18:55

ldionne approved these changes Jul 17, 2024

View reviewed changes

ldionne merged commit 7f2bd53 into llvm:main Jul 18, 2024
55 checks passed

robincaloudis mentioned this pull request Jul 21, 2024

[libc++][math] Fix acceptance of convertible types in std::isnan() and std::isinf() #98952

Merged

Harini0924 pushed a commit to Harini0924/llvm-project that referenced this pull request Jul 22, 2024

[libc++] Fix acceptance of convertible-to-{float,double,long double} …

da0cd38

…in std::isfinite() (llvm#98841) Closes llvm#98816.

sgundapa pushed a commit to sgundapa/upstream_effort that referenced this pull request Jul 23, 2024

[libc++] Fix acceptance of convertible-to-{float,double,long double} …

cf36e1d

…in std::isfinite() (llvm#98841) Closes llvm#98816.

robincaloudis mentioned this pull request Aug 28, 2024

[libc++][math] Remove constrained overloads of std::{isnan, isinf, isfinite} #106224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] Fix acceptance of convertible-to-{float,double,long double} in std::isfinite() #98841

[libc++] Fix acceptance of convertible-to-{float,double,long double} in std::isfinite() #98841

robincaloudis commented Jul 14, 2024

llvmbot commented Jul 14, 2024

github-actions bot commented Jul 14, 2024 •

edited

Loading

AngryLoki commented Jul 15, 2024 •

edited

Loading

ldionne commented Jul 15, 2024

robincaloudis commented Jul 16, 2024 •

edited

Loading

philnik777 left a comment

robincaloudis commented Jul 16, 2024

ldionne left a comment

ldionne commented Jul 16, 2024

ldionne left a comment

robincaloudis commented Jul 18, 2024

AngryLoki commented Jul 19, 2024

[libc++] Fix acceptance of convertible-to-{float,double,long double} in std::isfinite() #98841

[libc++] Fix acceptance of convertible-to-{float,double,long double} in std::isfinite() #98841

Conversation

robincaloudis commented Jul 14, 2024

llvmbot commented Jul 14, 2024

github-actions bot commented Jul 14, 2024 • edited Loading

AngryLoki commented Jul 15, 2024 • edited Loading

ldionne commented Jul 15, 2024

robincaloudis commented Jul 16, 2024 • edited Loading

philnik777 left a comment

Choose a reason for hiding this comment

robincaloudis commented Jul 16, 2024

ldionne left a comment

Choose a reason for hiding this comment

ldionne commented Jul 16, 2024

ldionne left a comment

Choose a reason for hiding this comment

robincaloudis commented Jul 18, 2024

AngryLoki commented Jul 19, 2024

github-actions bot commented Jul 14, 2024 •

edited

Loading

AngryLoki commented Jul 15, 2024 •

edited

Loading

robincaloudis commented Jul 16, 2024 •

edited

Loading