I'm reviewing this library as someone who has tried to use it in the past,
has made minor contributions to it and has a vested interest in improving
numeric support for C++ developers. However, I have very little knowledge
of the code and relatively little experience of Boost and the Boost
community. I hope my comments are helpful nevertheless.
tl;dr: make `safe<>` more like the type I describe in [
http://wg21.link/p0554#componentization-safety] or at least ensure that the
API can evolve toward this without backward-breaking changes.
----
I do feel that a safe integer type should be available to as many people as
possible and Robert's approach is on the right track. But given the choice,
I would make some significant API changes. Any of these which can be added
incrementally should not delay acceptance of safe_numerics into Boost.
The goal of making `safe<int>` a drop-in replacement for int is essential.
But further, `safe<Rep>` should be a drop-in replacement for `Rep` where
`Rep` is any integer type which exhibits "surprising" results, e.g. through
UB of overflow / divide-by-zero or indeterminate initial value. `Rep`
should not just be any type for which `std::is_integral_v<Rep>` is true and
ideally, `safe`, while pointless, should compile and function
like `safe<Rep>`. Unfortunately, `safe<int>` fails to be a drop-in
replacement for fundamental integer types for a number of reasons.
Firstly, conversion from floating-point types is not fully supported [
https://github.com/robertramey/safe_numerics/issues/13]. This alludes to a
wider issue with the API of this library: it tries to do too much.
Conversion from integers to real numbers is not so much *unsafe* as
*lossy*. Users intuitively understand that `int n = 0.5;` will not define a
variable with value 0.5 and this is often exactly what they intend, e.g.:
void sample(map& histogram, double v) {
++ histogram[v];
}
If implicit conversions to and from other numeric types is not supported,
this prevents potential users from converting existing programs. Bear in
mind that implicit conversions do not prevent `safe<>` from catching
run-time errors. And if one wishes to catch risky narrowing conversions,
they already have tools to do this, e.g. GCC's `-Wconversion`. In short,
it's a separate concern.
Another example of overreach of the API is in governing the results of
arithmetic operations. Via policy choice, `safe<int>` can produce
auto-widening functionality, e.g. returning a 16-bit result from a multiply
operation with 8-bit operands. This behavior belongs in a whole different
class. It achieves overlapping safety wins but it complicates the
interface. I believe that the correct approach is to produce a type which
is dedicated to performing run-time overflow checks and another type which
performs automatic widening and to combine them, e.g.:
// simplified for clarity, see:
wg21.link/p0554#composition-safe_elastic_integer
template<typename Rep> class auto_widening_integer;
template<typename Rep> class safe_integer;
template<typename Rep> using extra_safe_integer =
safe_integer;
Of the two component classes, `auto_widening_integer` can be used to
perform limited overflow-avoidance with zero run-time overhead. (The
penalty is that if, say, you multiply two 32-bit integers together, you get
back a 64-bit result.) Meanwhile, `safe_integer` provides iron-clad
run-time safety checking and obeys the promotion rules of its `Rep` type.
The combination of them reduces the number of run-time checks by reducing
the chance of overflow. This solution is more complex, but the individual
components are simpler and self-sufficient.
Unfortunately, due to these kinds of design choices, I have not spent more
time attempting to integrate Robert's library with my own. I would dearly
like to include `safe<>` as part of a wider set of numeric components which
users can then combine to their hearts content. Details of how I think that
might come about can be found in (shamelessly plugged) paper,
[wg21.link/p0554].
I accept that's a tall order. At the very least I would like developers of
substantial projects to be able to replace build-in integers with the
following:
#if defined(NDEBUG)
template<typename Rep> using Integer = Rep;
#else
template<typename Rep> using Integer = safe<Rep>;
#endif
Hope that helps,
John
