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(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
