[PATCH] Implement std::experimental::sample

Eric Fiselier eric at efcs.ca
Thu Apr 30 11:00:27 PDT 2015


More comments. The `__sample` that takes an input iterator and a random access iterator LGTM.


================
Comment at: include/experimental/algorithm:58
@@ +57,3 @@
+  for (; __first != __last; ++__first, (void)++__k) {
+    _Distance __r = _VSTD::uniform_int_distribution<_Distance>(0, __k)(__g);
+    if (__r < __sz)
----------------
`_Distance` is the wrong type to be using here. It's possible that `_Distance` is a type that cannot represent the value of `__k`. The LFTS spec seems misleading about how distance should be used. I'll have to file a standard defect to clarify the wording.

I think `__k` should be the `distance_type` of the `_PopulationIterator`.

Example Code (This has UB and exits with -6 on my machine).
```
std::vector<int> pop(1024, 42);
    std::vector<int> out(128, 0);
    std::minstd_rand g;
    std::experimental::sample(pop.begin(), pop.end(), out.begin(), (signed char)127, g);
```

================
Comment at: include/experimental/algorithm:74
@@ +73,3 @@
+    _Distance __r =
+        _VSTD::uniform_int_distribution<_Distance>(0, --__unsampled_sz)(__g);
+    if (__r < __n) {
----------------
Do we need to re-construct the `uniform_int_distribution` every iteration?

http://reviews.llvm.org/D9044

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the cfe-commits mailing list