Proof-of-Concept Implementation of D0205

[Jonathan Wakely]

On 10/02/16 20:07 +0100, Moritz Klammler wrote:
>I have implemented my proposal D0205 to allow seeding random number
>engines directly with `std::random_device` as in
>
>    std::random_device device {};
>    std::mt19937 engine {device};  // note: device passed by-reference
>    std::cout << "Here is a very random number: " << engine() << "\n";
>
>for libstdc++.  The paper will be officially submitted to ISO tomorrow
>and hopefully be discussed in Jacksonville and ideally accepted for
>C++17.  Attached is my patch against libstdc++ SVN revision 233225 and a
>quick note how to use it.  You can find the current version of any of
>these as well as the final D0205R1 of the paper on my website, too.
>
>    http://klammler.eu/data/computer-science/iso-c++/p0205/

Interesting, thanks for the patch and the link to the proposal.

>I would be very grateful for a review of my patch.  This is my first
>experience with writing standard library code.  By the way, I don't

I only did a quick review in terms of our coding conventions and
requirements, but it looks good: you've used reserved names everywhere
appropriate and you've added tests.

You should qualify the call to distance(__f, __l) to avoid some other
distance() being found by ADL in an associated namespace of the
iterators.

I'll leave a more complete review for after the Jacksonville meeting,
but some of our <random> experts might give you more feedback (on the
proposal itself as well as the picky details of our coding style etc.)

>know how to make the ABI compatibility check pass again, even though
>I've read the [ABI Policy and
>Guidelines](https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html).

Don't add the new exported symbol to a "closed" version. The
GLIBCXX_3.4.19 version has already been superseded by later versions,
so you cannot add new symbols to an old version.

>I didn't include a copyright notice in the files because I didn't want
>to say that something is by the FSF without being asked to do so.  I
>will be happy to fix this once asked for.

OK, until you assign copyright to the FSF you are the copyright
holder, so that makes sense.

For significant contributions like this we would need you to meet the
legal prerequisites linked to from
https://gcc.gnu.org/onlinedocs/libstdc++/manual/appendix_contributing.html#contrib.list

>Do you have any interest in incorporating this patch into libstdc++?  Is
>there a process for merging experimental features that are not
>officially standard yet?

In recent years we've tended to be more conservative about
implementing not-yet-standard features, because if they don't make it
into the standard we can be stuck having to support it for backwards
compatibility.

We have several non-standard engines in <ext/random>, but these are
generally pure additions that don't require any changes to the
standard engines and distributions (and it's in the spirit of the
C++11 <random> library to support additional engines that model the
standard concepts). I think we also have some non-standard member
functions for generating several random numbers more efficiently
than repeated calls to the operator() function. So we are certainly
willing to consider extensions when they make sense.

It's too late to make this change for GCC 6 now anyway (we're in
"stage 4" meaning we're only fixing regressions in preparation for the
release, not accepting new features). As a result I think we can wait
until your proposal is discussed in Jacksonville (I'll try to be
present for the discussion) and see if there is support for the idea
in principle, and if it undergoes major changes as a result of the
review. Then we can revisit your patch and decide what to do with it.


>In order to see the changes made by the patch, C++ files that want to make use
>of them must `#define _GLIBCXX_USE_P0205` before `#include`ing the `<random>`
>header.  There macro must be defined to either of the following values.
>
> - `#define _GLIBCXX_USE_P0205 'I'`
>   provides a header-only implementation that does not require re-building
>   `libstdc++`.  It adds all functionality but misses all optimizations.
>
> - `#define _GLIBCXX_USE_P0205 'L'`
>   provides an optimized implementation but requires re-building the
>   `libstdc++` library.

Although this is fine for a prototype if we accept this code into
libstdc++ we'll only want one or the other, not both. If it goes into
the standard I expect we'd want the optimized version that exports new
symbols. If we accept it as a non-standard extension it might be
better to use the header-only version so that we don't add symbols to
the shared library (which must then be supported until we change the
library's SONAME, which effectively means supporting it indefinitely).

>## Known Issues
>
> - This patch breaks `make check` for the ABI compliance  and I don't know how
>   to fix it.
>
> - The optimization deployed for iterators of type `std::uint32_t *` should
>   also be enabled for any other iterators of type `IterT` where
>   `std::iterator_traits<IterT>::value_type` is `std::uint32_t` and
>   `std::iterator_traits<IterT>::iterator_category` is
>   `std::contiguous_iterator_tag`, except that the latter does not exist yet.
>   Apparently, it is possible to do the optimization at least for standard
>   library iterators because `std::copy` et al do it too but I don't know how
>   to enable it.

Our vector and string use the __normal_iterator class template as
their iterator types. That's is a simple wrapper that behaves like a
raw pointer (so is a contiguous iterator) but can be instantiated to
produce a unique class type, so that std::string::iterator and
std::vector<char>::iterator are distinct types. To treat those
iterators as contiguous iterators you can either specialize/overload
on __normal_iterator or use the __niter_base() function in
<bits/stl_algobase.h> to "unwrap" it to get a raw pointer.

Thanks again for contributing your patch. If you don't hear anything
from us after your paper is discussed in Jacksonville then please send
another mail to this list, or ping me personally.