<random>: Remove one meaningless _Small_poisson_distribution::_Init call
#5442
Conversation
| _Sqrt = _CSTD sqrt(2 * _Mean * (1 - _Pp)); | ||
| _Logp = _CSTD log(_Pp); | ||
| _Logp1 = _CSTD log(1.0 - _Pp); | ||
| _Small._Init(_Mean); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh... the potentially buggy combination seems avoidable by renaming _Init. Although I have no idea whether it could really happen.
There was a problem hiding this comment.
If there is such problem, I don't believe that the renaming would suffice this time
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, this. And I mean the whole ..._distribution objjects.
There was a problem hiding this comment.
_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.)
|
Thanks for looking into this. Closing without merging as this is an ABI break that I see no way to cure without introducing "would |
github-project-automation
bot
moved this from Initial Review
to Done
in STL Code Reviews
3 participants
As shown by the comment below, the
_Smalldata member ofbinomial_distributionis unused, so it's unnecessary to call_Init. Given there's a default member initializer of_Small_poisson_distribution's_Gx0added in #5411, there won't be uninitialized object even when all explicit operations are skipped.Also changes the name of
binomial_distribution::param_type::_Initto_Init_v2to avoid potential mix-and-mismatch bugs.