Skip to content

Conversation

@StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Nov 20, 2025

Fixes #5577.

Overview

To implement <random>'s poisson_distribution and binomial_distribution, the STL had a "log gamma" function for historical reasons. As the UCRT provides C99 lgamma(), we should just call that. This will increase accuracy and reduce the amount of code we need to maintain.

Accuracy

I used Boost.Math's test coverage to compare UCRT lgamma() to STL _XLgamma(). Boost.Math's own implementation is the best (0 failures). UCRT lgamma() has minor accuracy issues (7 failures). STL _XLgamma() was extremely inaccurate (648 failures); @statementreply observed on Discord that this inaccuracy was around +/- 10^-9, which is millions of ULPs for double. Woof. 🐶

Click to expand the details of my hacked-up accuracy testing:

I used boost_1_89_0\libs\math\test\test_gamma.cpp and test_gamma.hpp. At the end of test_gamma.cpp, to make it header-only, I followed the docs and added:

#define BOOST_TEST_MODULE header-only multiunit test
#include <boost/test/included/unit_test.hpp>

In test_gamma.hpp I added:

#include <cmath>
#include <random>
float ucrt_lgamma(float val) {
    return std::lgamma(val);
}
double ucrt_lgamma(double val) {
    return std::lgamma(val);
}
long double ucrt_lgamma(long double val) {
    return std::lgamma(val);
}
float stl_xlgamma(float val) {
    return std::_XLgamma(val);
}
double stl_xlgamma(double val) {
    return std::_XLgamma(val);
}
long double stl_xlgamma(long double val) {
    return std::_XLgamma(val);
}

Results:

C:\Temp>cl /EHsc /nologo /W4 /wd4459 /std:c++latest /MTd /Od test_gamma.cpp /I. /I D:\GitHub\DELME\boost_1_89_0\libs\math\test /I D:\GitHub\DELME\boost_1_89_0 /DBOOST_TEST_NO_LIB /DBOOST_MATH_NO_REAL_CONCEPT_TESTS && test_gamma.exe
[...]
*** No errors detected

C:\Temp>cl /EHsc /nologo /W4 /wd4459 /std:c++latest /MTd /Od test_gamma.cpp /I. /I D:\GitHub\DELME\boost_1_89_0\libs\math\test /I D:\GitHub\DELME\boost_1_89_0 /DBOOST_TEST_NO_LIB /DLGAMMA_FUNCTION_TO_TEST=ucrt_lgamma /DBOOST_MATH_NO_REAL_CONCEPT_TESTS && test_gamma.exe
[...]
*** 7 failures are detected in the test module "Master Test Suite"

C:\Temp>cl /EHsc /nologo /W4 /wd4459 /std:c++latest /MTd /Od test_gamma.cpp /I. /I D:\GitHub\DELME\boost_1_89_0\libs\math\test /I D:\GitHub\DELME\boost_1_89_0 /DBOOST_TEST_NO_LIB /DLGAMMA_FUNCTION_TO_TEST=stl_xlgamma /DBOOST_MATH_NO_REAL_CONCEPT_TESTS && test_gamma.exe
[...]
*** 648 failures are detected in the test module "Master Test Suite"

Changes

<random>

Don't declare _XLgamma() anymore. (The all-important _CRTIMP2_PURE will be preserved on the definitions.)

Call _STD lgamma(), which is similarly overloaded for float / double / long double.

xlgamma.cpp

Transfer _CRTIMP2_PURE to the definitions, which will keep them dllexported. This replaces a silly pattern of "declare with _CRTIMP2_PURE but define without".

Mark them as preserved for bincompat.

Replace their guts with calls to _STD lgamma(), which preserves the interface but improves the behavior.

In `poisson_distribution` and `binomial_distribution`.
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner November 20, 2025 23:21
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Nov 20, 2025
@github-project-automation github-project-automation bot moved this to Initial Review in STL Code Reviews Nov 20, 2025
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in STL Code Reviews Nov 20, 2025
@JMazurkiewicz
Copy link
Contributor

I think we can close #5577 once this is merged.

@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in STL Code Reviews Nov 25, 2025
@StephanTLavavej StephanTLavavej moved this from Ready To Merge to Merging in STL Code Reviews Nov 25, 2025
@StephanTLavavej
Copy link
Member Author

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Something can be improved

Projects

Status: Merging

Development

Successfully merging this pull request may close these issues.

xlgamma.cpp: Is it possible to use UCRT's lgamma now?

3 participants