-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[libc++] Add a thread-safe version of std::lgamma in the dylib #153631
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,31 @@ inline _LIBCPP_HIDE_FROM_ABI double tgamma(_A1 __x) _NOEXCEPT { | |
return __builtin_tgamma((double)__x); | ||
} | ||
|
||
// __lgamma_r | ||
|
||
struct __lgamma_result { | ||
double __result; | ||
int __sign; | ||
}; | ||
|
||
#if _LIBCPP_AVAILABILITY_HAS_THREAD_SAFE_LGAMMA | ||
_LIBCPP_EXPORTED_FROM_ABI __lgamma_result __lgamma_thread_safe_impl(double) _NOEXCEPT; | ||
|
||
inline _LIBCPP_HIDE_FROM_ABI __lgamma_result __lgamma_thread_safe(double __d) _NOEXCEPT { | ||
return __math::__lgamma_thread_safe_impl(__d); | ||
} | ||
Comment on lines
+68
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason for having another wrapper here? |
||
#else | ||
// When deploying to older targets, call `lgamma_r` directly but avoid declaring the actual | ||
// function since different platforms declare the function slightly differently. | ||
double __lgamma_r_shim(double, int*) _NOEXCEPT __asm__("lgamma_r"); | ||
|
||
inline _LIBCPP_HIDE_FROM_ABI __lgamma_result __lgamma_thread_safe(double __d) _NOEXCEPT { | ||
int __sign; | ||
double __res = __math::__lgamma_r_shim(__d, &__sign); | ||
return __lgamma_result{__res, __sign}; | ||
} | ||
#endif | ||
|
||
} // namespace __math | ||
|
||
_LIBCPP_END_NAMESPACE_STD | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
//===----------------------------------------------------------------------===// | ||
// | ||
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
// See https://llvm.org/LICENSE.txt for license information. | ||
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#ifdef __APPLE__ | ||
# define _REENTRANT | ||
#endif | ||
|
||
#include <cmath> | ||
#include <math.h> // for lgamma_r | ||
|
||
_LIBCPP_BEGIN_NAMESPACE_STD | ||
namespace __math { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really want to enshrine the |
||
|
||
_LIBCPP_EXPORTED_FROM_ABI __lgamma_result __lgamma_thread_safe_impl(double __d) noexcept { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't hurt to have it though and we do have a bunch of functions in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have to say I completely disagree. IMO this significantly hurts readability. We should instead just have non-_Uglified names for library-internal functions (which itself helps with readability in general). Spotting exported ones is trivial in that case. Removing the attribute here also ensures that the correct declarations are visible at the definition, which helps avoid declaration mismatches. |
||
int __sign; | ||
double __result = ::lgamma_r(__d, &__sign); | ||
return __lgamma_result{__result, __sign}; | ||
} | ||
|
||
} // namespace __math | ||
_LIBCPP_END_NAMESPACE_STD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're never using the
__sign
. Should we remove that part? @lntue do you think this makes any difference in the implementation?