[PATCH] Fixed Noop insertion

[JF Bastien]

================
Comment at: lib/Support/RandomNumberGenerator.cpp:69
@@ +68,2 @@
+  return RandomNumber % Max;
+}
----------------
rinon wrote:
> rinon wrote:
> > jfb wrote:
> > > rinon wrote:
> > > > jfb wrote:
> > > > > Do you think you could create a utility that effectively meets the `uniform_int_distribution` requirements of the standard, while offering an implementation that doesn't change from one standard library to another? This would make it easier to convince readers of the code that the RNG+distribution is actually doing what they expect.
> > > > > 
> > > > > The obvious fix is to simply include [[ http://llvm.org/svn/llvm-project/libcxx/trunk/include/algorithm | libc++'s implementation ]] since the license is the same:
> > > > > ```
> > > > > template<class _IntType>
> > > > > template<class _URNG>
> > > > > typename uniform_int_distribution<_IntType>::result_type
> > > > > uniform_int_distribution<_IntType>::operator()(_URNG& __g, const param_type& __p)
> > > > > {
> > > > >     typedef typename conditional<sizeof(result_type) <= sizeof(uint32_t),
> > > > >                                             uint32_t, uint64_t>::type _UIntType;
> > > > >     const _UIntType _Rp = __p.b() - __p.a() + _UIntType(1);
> > > > >     if (_Rp == 1)
> > > > >         return __p.a();
> > > > >     const size_t _Dt = numeric_limits<_UIntType>::digits;
> > > > >     typedef __independent_bits_engine<_URNG, _UIntType> _Eng;
> > > > >     if (_Rp == 0)
> > > > >         return static_cast<result_type>(_Eng(__g, _Dt)());
> > > > >     size_t __w = _Dt - __clz(_Rp) - 1;
> > > > >     if ((_Rp & (_UIntType(~0) >> (_Dt - __w))) != 0)
> > > > >         ++__w;
> > > > >     _Eng __e(__g, __w);
> > > > >     _UIntType __u;
> > > > >     do
> > > > >     {
> > > > >         __u = __e();
> > > > >     } while (__u >= _Rp);
> > > > >     return static_cast<result_type>(__u + __p.a());
> > > > > }
> > > > > ```
> > > > > The duplication is unfortunate, but generating the same sequence independently of stdlib implementation kind of makes duplication a requirement.
> > > > Sorry, I missed your comment. Somehow it slipped by my email.
> > > > 
> > > > Yes, I think we could use libc++'s implementation (from the spec it seems that independent_bits_engine should be stable). However, I think the only thing we really gain with this is efficiency. The libc++ implementation is faster, at the expense of readability and code size.
> > > > 
> > > > I'm happy to integrate this optimized implementation into the current RNG class, but I'd be adverse to creating a completely duplicate uniform_int_distribution utility class. That seems like a lot of code duplication for a simple feature.
> > > My main concern is convincing myself that your implementation indeed has the advertised uniform distribution. When I read the code I don't find this obvious, and there's no unit test for this function to check the distribution is indeed uniform.
> > > 
> > > It's indeed even less obvious from the libc++ implementation, but having the same implementation means that we can rely on libc++'s {test suite / users / following the spec} to make sure it's correct. AFAICT libc++ doesn't have a unit test on its distribution either...
> > On closer inspection, we can't use the libc++ implementation. It depends on independent_bits_engine taking the number of bits as a constructor parameter, not as a template parameter as the standard dictates. Since the range should be a parameter, this makes life difficult.
> > 
> > I'll see if I can't find a solid reference or implementation so we don't need to worry about this bit. It really shouldn't be bad.
> If you want a "more-tested" implementation, how would you feel about using OpenBSD's arc4random_uniform implementation (http://cvsweb.openbsd.org/cgi-bin/cvsweb/~checkout~/src/lib/libc/crypt/arc4random_uniform.c?rev=1.1&content-type=text/plain)? It's BSD licensed and a super simple uniform distribution.
I'd make sure it's initialized first :-p

But sure, as long as the implementation is license-compatible and well tested then that sounds acceptable.

http://reviews.llvm.org/D6983

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