Re: [boost] [safe_numerics] review

Subject: Re: [boost] [safe_numerics] review
From: Robert Ramey (ramey_at_[hidden])
Date: 2017-03-10 19:59:14

On 3/10/17 10:55 AM, John McFarlane via Boost wrote:
> 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.

Thank you for taking the time to do this.
>
> 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.

from your link:
The integral (signed) and cardinal (unsigned) class templates from 
[P0106] perform such a role using non-type template parameters, e.g.

template<int Crng, overflow Covf = overflow::exception>
class integral;
where Crng specifies the range of the value in bits and overflow is an 
enum which specifies what happens if overflow is detected. The values of 
overflow include:

undefined - much like signed integer overflow behavior;
abort - forces program termination;
exception - throws an exception;
saturate - limits the value to the maximum/minimum allowed by the range.

To my mind, this seems very very similar to my own formulation

template<
        class T,
        class PP = native,
        class EP = throw_exception.
struct safe;

EP can support policies equivalent to undefined, abort, throw exception 
as well as trap at compile time.  So as I said - it's to me they are 
almost exactly the same interfaces.

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

I'm all ears.  One thing I do NOT see is saturate.  I've seen it 
mentioned in several places - but I'm not convinced.

> The goal of making `safe<int>` a drop-in replacement for int is essential.
Agreed

> 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

This is my intention.  The requirements that for type T it be supported 
by numeric_limits<T> with a number of settings - is_specialized, 
is_integer, max(), min() (or lowest). etc. BTW safe<T> also should be 
supported if T meets these requirements.  I haven't tested this though. 
I might bump up against some implementation oversight.

> ideally, `safe<safe<Rep>>`, while pointless, should compile and function
> like `safe<Rep>`.
Maybe - from a completeness point of view, but I don't see any practical 
usage for such a type.

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].

I'm not sure this is still true.  I did make changes in response to this 
issue, but I haven't exhaustively tested inter operability with floating 
point types.  If it doesn't it's a bug - not a design issue.

> 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.:

Current behavior is:

safe<int> n;
n = 0.05 // will trap at compile time with error
n = static_cast<int>(0.05); // will compile OK

My personal belief is that this is correct behavior.

Note that I'm not a saint. I DO permit n to be uninitialized.  I did 
this because it's so common and I want the library to be useful on 
legacy and embedded code.  I've got a lot of reservations about this and 
could change my mind.

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

The library is meant to support implicit conversions which do not change 
arithmetic values. I do believe that your example will work. See the 
example in the tutorial section labeled "Programming by Contract is Too 
Slow"

your example includes ++historgram[v] where v is a double.  My version 
would choke on that- as I think it should.

> 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.
This isn't portable and (I believe results in a compile time error). 
Limited utility as far as I'm concerned.

> 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,
agreed

 > but the individual components are simpler and self-sufficient.
maybe

I'm a great believe in de-coupling.  But I'm skeptical that a library 
with this functionality can be realised in the manner outlined above.

I also think this approach would conflict with the goal of "almost 
drop-in replaceability"

I have factored out the functions of type promotion and error handling 
into the policy classes for which there are obvious default 
implementations.  And the library includes most required policies so 
that users won't have to write their own.  I believe that it puts the 
customization in the right place.

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

We've factored things in a different way.  So I'm not convinced that 
there is a way to compose safe numerics and the ideas outlined in the 
papers you sight is a meaningful way.

My conclusion after looking at the papers is that we're all attempting 
to address the same important problem in different ways.  My way places 
high priority in handling legacy code.  The other way seems more 
appropriate for use in creating new code which is meant to support 
correctness from the start - building in quality rather than trying to 
add it on as I do.

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

That is certainly my intention. I believe this usage is well supported 
by the safe numerics library.

> Hope that helps,
It does.  Thanks for your review.

> John
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
>