[libcxx-commits] [PATCH] D74997: [libc++] Bugfix to std::binomial_distribution<int>

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

In D74997#1918498 <https://reviews.llvm.org/D74997#1918498>, @atmnpatel wrote:

> Sorry, I was transitioning between machines while updating the commit and I couldn't figure out arc diff - I'd like it to be committed under a335pate at uwaterloo.ca.


Ok, will do.

> [...]
> 
> If we assume that the current implementation is in fact a functional implementation of the binomial_distribution, this edit will not change that fact, and will only return 0 in the cases where the current implementation will not terminate. Also I'd like to be really upfront about this - It returns 0 as opposed to following the paper in the catch-all return condition because if the paper is followed, the mean is nowhere near it should be, and this is particular evident in the case of the bug when the probability is set to be extremely small. [...]

Ok, reading your explanation, I believe I follow. Thanks for explaining. I'm fine with the change.

In D74997#1918581 <https://reviews.llvm.org/D74997#1918581>, @EricWF wrote:

> Could you please upload this diff with more context?


@atmnpatel  What Eric meant here is to upload a patch with some Diff context. For example, see https://reviews.llvm.org/D76288 where you can expand/collapse the code surrounding the changes, and where it says instead `Context not available`. We prefer reviewing patches with Diff context cause it's easier, but this time around it's okay.

Thanks a lot for your contribution, I'm going to commit this for you now under a335pate at uwaterloo.ca.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74997/new/

https://reviews.llvm.org/D74997