Implement LWG-3809 Is subtract_with_carry_engine<uint16_t> supposed to work?
#4194
Conversation
supposed to work?
stl/inc/random
Outdated
| void seed(_Seed_t _Value = default_seed, bool _Readcy = false) { // set initial values from specified seed value | ||
| linear_congruential<_Seed_t, 40014U, 0U, 2147483563U> _Lc{_Value == 0U ? default_seed : _Value}; | ||
| linear_congruential_engine<uint_least32_t, 40014U, 0U, 2147483563U> _Lc{ | ||
| static_cast<uint_least32_t>(_Value == 0U ? default_seed : _Value)}; | ||
| _Reset(_Lc, _Readcy); | ||
| } |
There was a problem hiding this comment.
Hi @jwakely. Sorry to bother you, but I think you provided the wording for LWG-3809 "Is std::subtract_with_carry_engine<uint16_t> supposed to work?" This part causes the seed value to be truncated to at least 32 bits (maybe 32, maybe more). Is that intentional? It causes the results to change for wider types and also makes it non-portable because the amount of truncation isn't specified. We'd appreciate if you could provide some insight. Thanks!
There was a problem hiding this comment.
The seed value will be reduced mod 2147483563 anyway, and the subtract_with_carry_engine uses the LCG engine to produce n 32-bit values (where n is the number of 32-bit pieces needed for a word of size w), so a 32-bit LCG is the right choice. Using a larger seed is possible, but has no benefit.
I suppose we could do value % 2147483563 before the conversion to UIntType:
linear_congruential_engine<uint_least32_t,
40014u,0u,2147483563u> e(value == 0u ? default_seed : value % 2147483563u);That would remove any dependence on the width of uint_least32_t.
There was a problem hiding this comment.
Thanks for your comment. I can write a new LWG issue explaining the situation and proposing that fix, unless there's a better way to handle it.
There was a problem hiding this comment.
For reference: LWG-4014.
|
Thanks! I pushed very minor changes to the tr1 codepaths to make them consistent with the Standard codepaths, which I think will be far less confusing. I think this needs only one maintainer approval. |
| void seed(_Seed_t _Value = 0u, bool _Readcy = false) { // set initial values from specified seed value | ||
| linear_congruential_engine<uint_least32_t, 40014U, 0U, 2147483563U> _Lc{ | ||
| static_cast<uint_least32_t>(_Value == 0U ? default_seed : _Value)}; |
There was a problem hiding this comment.
Style:
Case inconsistency between u and U literals, or it doesn't matter?
There was a problem hiding this comment.
It seems that the standard wording consistently uses lowercase (u) in [rand], while MSVC STL consistently uses uppercase (U)...
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
|
Thanks for subtracting 1 from the number of remaining LWG issues! 😹 🎉 📉 |
|
Only until the new one is opened ;-) |
Fixes #4162.
Also updates citations in the modified test file.