Skip to content

<random>: Remove one meaningless _Small_poisson_distribution::_Init call #5442

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

Closed
frederick-vs-ja wants to merge 2 commits into from
Closed

<random>: Remove one meaningless _Small_poisson_distribution::_Init call #5442

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e14b49
Remove one meaningless `_Small_poisson_distribution::_Init` call
frederick-vs-ja Apr 26, 2025
ea30bae
Rename `_Init` to `_Init_v2` to avoid potential link-time bugs
frederick-vs-ja Apr 26, 2025
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
9 changes: 4 additions & 5 deletions stl/inc/random
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2941,11 +2941,11 @@ public:
using distribution_type = binomial_distribution;

param_type() noexcept /* strengthened */ {
_Init(1, _Ty1(0.5));
_Init_v2(1, _Ty1(0.5));
}

explicit param_type(_Ty _Tx0, _Ty1 _Px0 = _Ty1(0.5)) noexcept /* strengthened */ {
_Init(_Tx0, _Px0);
_Init_v2(_Tx0, _Px0);
}

_NODISCARD friend bool operator==(const param_type& _Left, const param_type& _Right) noexcept
Expand All @@ -2968,7 +2968,7 @@ public:
return _Px;
}

void _Init(_Ty _Tx0, _Ty1 _Px0) noexcept { // initialize
void _Init_v2(_Ty _Tx0, _Ty1 _Px0) noexcept { // initialize
_STL_ASSERT(0.0 <= _Tx0, "invalid max argument for binomial_distribution");
_STL_ASSERT(0.0 <= _Px0 && _Px0 <= 1.0, "invalid probability argument for binomial_distribution");
_Tx = _Tx0;
Expand All @@ -2979,7 +2979,6 @@ public:
_Sqrt = _CSTD sqrt(2 * _Mean * (1 - _Pp));
_Logp = _CSTD log(_Pp);
_Logp1 = _CSTD log(1.0 - _Pp);
_Small._Init(_Mean);
Copy link
Contributor

@AlexGuteniev AlexGuteniev Apr 26, 2025

Choose a reason for hiding this comment

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

I'm not sure. If it was used in some before version, and there's compiled code with that STL that takes already constructed distribution, there will be a bug.

Copy link
Contributor Author

@frederick-vs-ja frederick-vs-ja Apr 26, 2025

Choose a reason for hiding this comment

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

Oh... the potentially buggy combination seems avoidable by renaming _Init. Although I have no idea whether it could really happen.

Copy link
Contributor

@AlexGuteniev AlexGuteniev Apr 26, 2025

Choose a reason for hiding this comment

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

If there is such problem, I don't believe that the renaming would suffice this time

Copy link
Contributor Author

@frederick-vs-ja frederick-vs-ja Apr 27, 2025

Choose a reason for hiding this comment

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

I'm not sure what the exact problem is. Did you mean that newly created _Small subobjects will still be processed by legacy compiled code, so data consistency (not only function definitions) needs to be kept?

Copy link
Contributor

@AlexGuteniev AlexGuteniev Apr 27, 2025

Choose a reason for hiding this comment

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

Yes, this. And I mean the whole ..._distribution objjects.

Copy link
Member

@StephanTLavavej StephanTLavavej Apr 28, 2025

Choose a reason for hiding this comment

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

_Small became unused after @MattStephanson's #1531 was merged on 2021-02-12 in VS 2019 16.10.

Renaming _Init to _Init_v2 helps prevent one category of mix-and-match problems. @AlexGuteniev is additionally correct that it doesn't entirely prevent problems. Consider the scenario where a new binary (e.g. user.exe) is compiled with this PR (18.0+) and passes a binomial_distribution to a very old binary (e.g. library.dll) that was compiled pre-#1531 (VS 2019 16.9 or earlier, including all VS 2017 and VS 2015). Then the new binary won't initialize _Small, but the very old binary will try to use it.

_Small_poisson_distribution::_Init sets _Gx0 = _CSTD exp(-_Mean0); so the new data member initializer of _Ty1 _Gx0 = 0.0; is not sufficient.

On the one hand, this is a pretty big span of time - users would have to be mixing code across 4+ years. As long as nothing older than the last supported release of VS 2019 16.11.x is used (including all VS 2022), then #1531 is always present. And it seems uncommon for a binomial_distribution to be passed around, or even stored as a data member; it's much more likely to be used as a local variable, which makes mismatch harder.

On the other hand, this is technically an ABI break, and it's not a major performance win. The risk is (IMO) quite small but the reason to take the risk is also small. We could very very likely get away with it (99.99%) but since we've noticed it, I think the correct thing to do is to avoid merging this.

If this increased correctness, with the same low risk of mix-and-match issues, then I would say that the correctness fix would be worth the risk, but not for a minor performance improvement.

(I observe that #1531 was a correctness improvement itself, so any pre-#1531 code that was activating the _Small codepath wasn't getting very good quality, and really ought to be rebuilt with #1531. However, "low quality" is different from "utterly broken". If pre-#1531 code were utterly broken then I wouldn't worry about mismatch here.)

}

_Ty _Tx;
Expand Down Expand Up @@ -3058,7 +3057,7 @@ public:
binomial_distribution::_Ty1 _Px0;
_In(_Istr, _Px0);
_In(_Istr, _Tx0);
_Dist._Par._Init(_Tx0, _Px0);
_Dist._Par._Init_v2(_Tx0, _Px0);
return _Istr;
}

Expand Down