microsoft / STL

MSVC's implementation of the C++ Standard Library.
Other
9.88k stars 1.45k forks source link

Implement P0952R2: 'A new specification for `std::generate_canonical`' #4740

Closed MattStephanson closed 1 day ago

MattStephanson commented 1 week ago

Fixes #1074. Fixes #4169.

MattStephanson commented 1 week ago

The failed test checks the internal helper function _Generate_canonical_iterations, which I've removed and replaced with a version that calculates more parameters at compile time. I could either adapt the test to the new implementation or just remove the test entirely. I'm leaning toward removing it because (1) testing internal function is generally undesirable and (2) the test was testing a naive implementation of Standard wording that no long exists ( $k = \mathrm{max}(1, \lceil b / log_2 R\rceil)$ ) after WG21-P0952. I'd welcome any comment on this point.

StephanTLavavej commented 1 week ago

I'm leaning toward removing it because (1) testing internal function is generally undesirable and (2) the test was testing a naive implementation of Standard wording that no long exists

Removing it sounds good to me.

StephanTLavavej commented 3 days ago

Thanks, this is awesome! :heart_eyes_cat:

StephanTLavavej commented 3 days ago

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

MattStephanson commented 2 days ago

Thanks Stephan! I have a few follow-up comments, but I'm at work right now and can't sign in to GitHub. Would you mind keeping this open until I can respond?

On Wed, Jun 26, 2024 at 12:06 AM Stephan T. Lavavej < @.***> wrote:

Thanks, this is awesome! 😻

— Reply to this email directly, view it on GitHub https://github.com/microsoft/STL/pull/4740#issuecomment-2190972024, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQOILAGZFU27T7BBW5D7AQDZJJR7FAVCNFSM6AAAAABJTBXT76VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJQHE3TEMBSGQ . You are receiving this because you authored the thread.Message ID: @.***>

StephanTLavavej commented 2 days ago

Sure thing, I'll hold off on merging this until you can respond. If I messed up something that should be fixed before merging instead of addressed in a followup, I can easily pull this from the merge batch for rework. Thanks! :heart_eyes_cat:

StephanTLavavej commented 2 days ago

Looks like there are /clr:pure timeouts (after 10 minutes) for GH_001017_discrete_distribution_out_of_range, possibly involving the 128-bit dependency, so I'll need to investigate that and push a commit later.

StephanTLavavej commented 1 day ago

Thanks for implementing this major overhaul of central <random> machinery! :gear: :hammer_and_wrench: :green_heart: