[libc++] Fix return type of ilogb(double)

[steffenlarsen]

This commit addresses a seemingly unintentional return type of the ilogb overload taking a double. Currently it returns double, while it is supposed to return int.

The return types seems to be covered by
libcxx/test/std/numerics/c.math/cmath.pass.cpp, so it is unclear how this discrepancy has gone undetected.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Steffen Larsen (steffenlarsen)

Changes

This commit addresses a seemingly unintentional return type of the ilogb overload taking a double. Currently it returns double, while it is supposed to return int.

The return types seems to be covered by
libcxx/test/std/numerics/c.math/cmath.pass.cpp, so it is unclear how this discrepancy has gone undetected.

---

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

2 Files Affected:

• (modified) libcxx/include/__cxx03/__math/logarithms.h (+1-1)
• (modified) libcxx/include/__math/logarithms.h (+1-1)

diff --git a/libcxx/include/__cxx03/__math/logarithms.h b/libcxx/include/__cxx03/__math/logarithms.h
index 25473501435cc..9b9e59a5a7baf 100644
--- a/libcxx/include/__cxx03/__math/logarithms.h
+++ b/libcxx/include/__cxx03/__math/logarithms.h
@@ -58,7 +58,7 @@ inline _LIBCPP_HIDE_FROM_ABI double log10(_A1 __x) _NOEXCEPT {
 inline _LIBCPP_HIDE_FROM_ABI int ilogb(float __x) _NOEXCEPT { return __builtin_ilogbf(__x); }
 
 template <class = int>
-_LIBCPP_HIDE_FROM_ABI double ilogb(double __x) _NOEXCEPT {
+_LIBCPP_HIDE_FROM_ABI int ilogb(double __x) _NOEXCEPT {
   return __builtin_ilogb(__x);
 }
 
diff --git a/libcxx/include/__math/logarithms.h b/libcxx/include/__math/logarithms.h
index 5f5f943977a50..7343d6a84ad4b 100644
--- a/libcxx/include/__math/logarithms.h
+++ b/libcxx/include/__math/logarithms.h
@@ -58,7 +58,7 @@ inline _LIBCPP_HIDE_FROM_ABI double log10(_A1 __x) _NOEXCEPT {
 inline _LIBCPP_HIDE_FROM_ABI int ilogb(float __x) _NOEXCEPT { return __builtin_ilogbf(__x); }
 
 template <class = int>
-_LIBCPP_HIDE_FROM_ABI double ilogb(double __x) _NOEXCEPT {
+_LIBCPP_HIDE_FROM_ABI int ilogb(double __x) _NOEXCEPT {
   return __builtin_ilogb(__x);
 }
 

[frederick-vs-ja]

I don't think we should touch frozen C++03 headers unless there's a critical issue.

[steffenlarsen]

Fine by me, but should we at least add a comment noting the observation of the bug? Or maybe there is a more appropriate place to track it for good measure?

[philnik777]

We can merge this into the C++03 headers.

[frederick-vs-ja]

Per the related test file, it seems that this overload is never selected.

llvm-project/libcxx/test/std/numerics/c.math/cmath.pass.cpp

Line 1234 in 75e60e7

static_assert((std::is_same<decltype(std::ilogb((double)0)), int>::value), "");

IIUC on every platform where libc++ is supported, ilogb(0.0)/std::ilogb(0.0) selects the ilogb function provided by the underlying C standard library. I wonder whether we can just remove this overload (and many overloads for double in <cmath>).

Also, __math::ilogb is not used within libc++, although some other functions in __math are used internally. So this overload is not used at all. I'm not sure whether we should fix the return type or remove this overload (together with some other templated function overloads for double).

[steffenlarsen]

IIUC on every platform where libc++ is supported, ilogb(0.0)/std::ilogb(0.0) selects the ilogb function provided by the underlying C standard library. I wonder whether we can just remove this overload (and many overloads for double in <cmath>).

I suppose this may be the reason for the templating. If there is non-templated ilogb(double), that would take priority over this, which may explain why the cmath.pass.cpp test typically doesn't fail. However, could this still be useful in cases where libc isn't found? Can that happen?

[philnik777]

The intent here is that if the libc doesn't provide these overloads the full overload set is still provided. I don't think we should remove this overload, since that would most likely result in weird bugs down the road if this function is ever used as __math::ilogb. I'm tempted to just merge this patch as-is.

[ldionne]

IIUC then the test would fail if a libc didn't provide the double overload, correct? We just happen not to have test coverage for that.

[philnik777]

Yes, exactly.