|
Boost : |
Subject: [boost] [safe_numerics] review
From: John McFarlane (john_at_[hidden])
Date: 2017-03-10 18:55:44
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<safe<Rep>>`, 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<safe<int>, safe<int>>& 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<auto_widening_integer<Rep>>; 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
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk