Re: [boost] [safe_numerics] review

Subject: Re: [boost] [safe_numerics] review
From: John McFarlane (john_at_[hidden])
Date: 2017-03-11 04:29:03

On Fri, Mar 10, 2017 at 11:59 AM Robert Ramey via Boost <
boost_at_[hidden]> wrote:

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

This probably isn't clear enough in my paper but P0106 is Lawrence Crowl's
fixed-point paper - not mine. In general, I argue for more type template
parameters and fewer non-type template parameters. I would argue that `EP`
is a "necessary evil" - rather like the second parameter of `std::vector`.
However, `EP` should be implicitly specified by `T`. `T` already does
whatever promotion `T` does, so to speak. So just do whatever naked `T`
would do WRT promotion.

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

You mean you don't see the need for it? It comes up quite often as a
strategy for dealing with overflow and a safe integer type seems like a
good place to house it. Even it it isn't implemented now, a policy
interface which allows users to implement it is going to give the library
more users who use it to make their integers safe - surely the point of the
exercise here.

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

I would make both choices differently. Because you can assign 0.05 to int,
you should also be able to assign 0.05 to safe<int> or else it's inherently
not a drop-in replacement. And of the list of things that makes fundamental
scalar types unsafe, indeterminate value of default-initialized instances
ranks higher than either overflow or divide-by-zero in my experience.
Fixing this would in no way break legacy code. It will impose a run-time
cost but normally this will be optimized out.

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

That is where we disagree.

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

Not if the conversion uses `static_cast`: https://godbolt.org/g/13aaO3

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

I draw the opposite conclusion but I'm glad we both have the same aim to
provide a low-resistance path to safe arithmetic.

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

My pleasure. Good luck!

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