[boost] [safe_numerics] One more review

Subject: [boost] [safe_numerics] One more review
From: Antony Polukhin (antoshkka_at_[hidden])
Date: 2017-03-11 09:00:07

- What is your evaluation of the design?

Looks good, simple to use.

I'd like to see an additional statefull ExceptionPolicy that remembers
that an UB was triggered, but does not throw at all. Here's how it
could be used:

bool try_do_something(int& out) noexcept {
    using namespace boost::numeric;
    safe<int, native, lazy> x = INT_MAX;
    // remembers that error was triggered
    ++x;
    // ... other operations on `x`
    // ...

    if (!x.no_errors()) {
        // One of the operations triggered UB
        return false;
    }

    out = x;
    return true;
}

- What is your evaluation of the implementation?

Checked a few border cases that I'm aware of. All of them were taken
care of in the library.

Nitpicks:

Missing BOOST_NORETURN in multiple places, for example
https://github.com/robertramey/safe_numerics/blob/master/include/exception_policies.hpp#L75
. Adding that attribute may result in better code generation and in
warnings suppression.

std::is_literal_type type trait is deprecated since C++17
https://github.com/robertramey/safe_numerics/blob/master/include/interval.hpp#L38

More generic streaming operators could be profitable (better to use
basic_ostream<Char, Trait> instead of ostream)
https://github.com/robertramey/safe_numerics/blob/master/include/interval.hpp#L448

Many operations may be decorated with noexcept

This macro seems broken
https://github.com/robertramey/safe_numerics/blob/master/include/safe_literal.hpp#L142
. Also looks like this class is not covered well with tests, because
line https://github.com/robertramey/safe_numerics/blob/master/include/safe_literal.hpp#L90
shall not compile

There's a bunch of implicit conversions in the library
https://github.com/robertramey/safe_numerics/blob/master/include/safe_base.hpp#L224
(BTW, have not found a body of this one). Would it be better to make
them explicit?

Those headers are very heavy
https://github.com/robertramey/safe_numerics/blob/7ee656ecdf23f6f29f93331769ba2209cb400be3/include/checked_result.hpp#L137-L138
Try using <iosfwd> and making all the function below to use
`basic_ostream<Char, Trait>&`.

No need to define those in `namespace std`
https://github.com/robertramey/safe_numerics/blob/7ee656ecdf23f6f29f93331769ba2209cb400be3/include/checked_result.hpp#L140
. Defining those in `boost::numeric` namespace works fine (thanks to
ADL!).

I'd really like to see tests coverage for the library. This is pretty
simple to add, just follow the instructions from here:
https://svn.boost.org/trac/boost/wiki/TravisCoverals

- What is your evaluation of the documentation?

Good, very interesting examples.

- What is your evaluation of the potential usefulness of the library?

Useful. I was dealing with such problems in Boost.LexicalCast.

- Did you try to use the library? With what compiler? Did you have any problems?

Have not tried.

- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

About two hours of reading source code and docs.

- Are you knowledgeable about the problem domain?

A little bit, not an expert.

- Do you think the library should be accepted as a Boost library?

Yes, the library should be accepted.

-- 
Best regards,
Antony Polukhin