Skip to content

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

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.

Sign up for GitHub

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
Jul 31, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libcxx/include/__cxx03/__math/logarithms.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return __builtin_ilogb(__x);
}

Expand Down
2 changes: 1 addition & 1 deletion libcxx/include/__math/logarithms.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, exactly.

return __builtin_ilogb(__x);
}

Expand Down
Loading