[boost] [Safe Numerics] Review

Subject: [boost] [Safe Numerics] Review
From: John Maddock (jz.maddock_at_[hidden])
Date: 2017-03-11 19:37:46

OK, here's my review of Robert's library.

By way of something different, I've tried to not read the docs (though I 
ended up scanning them in the end) and just tried to use the library.  
Here's what I've found:

1) Using:

safe<int>(-1) & 1

I get a hard assert failure at safe_base_operations.hpp:1373.  I'm 
assuming that this is a mistake, and the usual error handlers should be 
called?  Bitwise or has the same issue.  More tests required ;)

2) I'm not sure if this is an msvc issue but in:

constexpr safe_base operator~()

The static assert is triggered for *unsigned types* and not for signed.  
Even if this is a compiler issue, it indicates a missing test case or two.

3) If I assign a float to an integer, then I get the error: "conversion 
of integer to float loses precision" which isn't quite what you meant to 
say.
More particularly, for float conversions I would expect:

* Conversion from a float holding an integer value that's within the 
range of the target type to always succeed.
* Conversion from a float holding a non-integral value to conditionally 
succeed (with trunction) depending on the policy in effect.
* Conversion *to* a float may also fail if the integer is outside the 
range of the float (no checking may be required if the number of bits in 
the integer is less than the number of bits in the float).

4) Is constexpr arithmetic supposed to be supported? I tried:

     constexpr boost::numeric::safe<int> j = 0;
     constexpr boost::numeric::safe<int> k = 3;
     constexpr boost::numeric::safe<int> l = j + k;

Which generates an internal compiler error for msvc, and for gcc-5.3 I get:

../../../include/safe_base_operations.hpp:332:35: error: call to 
non-constexpr function ‘void boost::numeric::dispatch(const 
boost::numeric::checked_result<R>&) [with EP = 
boost::numeric::throw_exception; R = int]’
          dispatch<exception_policy>(r);

Similarly if I try to add a literal to a safe<int>.  Again as the tests 
all pass, so I'm assuming they're missing constexpr tests?

5) What is the purpose of class safe_literal? constepxr initialization 
seems to work just fine without it?

6) In the docs, the example of using error handler dispatch is nonesense 
(top of page 41)?

7) Related to the above, I see the need to be able to define your own 
primitives which behave just like yours.
The example I have is for bit-scan-left/right which has as precondition 
that the argument is non-zero, but there are other bit-fiddling 
operations (bit count etc) which some folks may require.  A "cookbook" 
example would be good.

8) In the tests, you have an undocumented function "log" in the 
library.  Not sure if this is intended to be part of the API or not, but 
IMO it's misnamed.  ilog2 seems to a semi-standard name for this.

9) By way of testing the libraries performance I hooked it into the 
Millar-Rabin primality test in the multiprecision lib (test attached, it 
counts how many probable primes there are below 30000000), here are the 
times for msvc:

Testing type unsigned:
time = 17.6503 seconds
count = 1857858
Testing type safe<unsigned>:
time = 83.5055 seconds
count = 1857858
Testing type 32-bit cpp_int:
time = 36.9026 seconds
count = 1857858

and for GCC-MINGW-5.3.0:

Testing type unsigned:
time = 17.1037 seconds
count = 1857858
Testing type unsigned:
time = 18.4377 seconds
count = 1857858
Testing type unsigned:
time = 21.0592 seconds
count = 1857858

I'm surprised by the difference in times between msvc and gcc, also 
deeply impressed that there is effectively no performance penalty with 
GCC (I was expecting there should be some!)

10) I don't find the scope of the library over-broad, in fact I probably 
find it about right wrt the policies etc.

11) There were a few traits and such that it would be nice to have, the 
problem as ever, is what to call them and where to put them:

* A trait for extracting the underlying type of a safe<> - use case for 
example where you enable_if a function on is_safe so don't have the 
template argument list to safe<>.
* Do we need a function for extracting the underlying type of a safe<>?  
Or is casting to the underlying type optimal? Actually, maybe having a 
function would negate the need
for the above trait, as we could just write 'auto x = mysafe.value();'  ?
* Do we need traits equivalent to 
is_signed/is_unsigned/make_signed/make_unsigned?  My gut feeling is yes 
we do, the issue is what to call them, and where to put them -
we're not allowed to specialize the type_traits versions (either boost 
or std), but we would need a central "one true trait" version that can 
be specialized by other libraries
as well (Multiprecision for example).  We've backed ourselves into a 
corner over this I fear!

12) When calling boost::integer::gcd/lcm the sub-optimal(?) non-binary 
versions are called for safe<> - you might want to look into seeing what 
traits
need to be defined to ensure that the same version as the underlying 
type is called.

Those are all the issues I can see for now... in conclusion, as for 
whether the library should be accepted, yes I believe it should be, 
modulo the issues above.

Regards, John.

---
This email has been checked for viruses by AVG.
http://www.avg.com