[safe_numerics] One more review

Antony Polukhin

11 Mar 2017 11 Mar '17

9 a.m.

- 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 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_p... . 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.hp... More generic streaming operators could be profitable (better to use basic_ostream instead of ostream) https://github.com/robertramey/safe_numerics/blob/master/include/interval.hp... Many operations may be decorated with noexcept This macro seems broken https://github.com/robertramey/safe_numerics/blob/master/include/safe_litera... . Also looks like this class is not covered well with tests, because line https://github.com/robertramey/safe_numerics/blob/master/include/safe_litera... shall not compile There's a bunch of implicit conversions in the library https://github.com/robertramey/safe_numerics/blob/master/include/safe_base.h... (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/7ee656ecdf23f6f29f93331769... Try using <iosfwd> and making all the function below to use `basic_ostream&`. No need to define those in `namespace std` https://github.com/robertramey/safe_numerics/blob/7ee656ecdf23f6f29f93331769... . 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

Show replies by date

Steven Watanabe

11 Mar 11 Mar

3:52 p.m.

AMDG On 03/11/2017 02:00 AM, Antony Polukhin via Boost wrote:

...

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:

This also requires an extra function to combine two ExceptionPolicies for binary operators. You'll run into problems with comparison operators, though, as a bool can't hold an ExceptionPolicy. In Christ, Steven Watanabe

Robert Ramey

5:22 p.m.

On 3/11/17 7:52 AM, Steven Watanabe via Boost wrote:

...

AMDG

On 03/11/2017 02:00 AM, Antony Polukhin via Boost wrote:

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

This also requires an extra function to combine two ExceptionPolicies for binary operators. You'll run into problems with comparison operators, though, as a bool can't hold an ExceptionPolicy.

Couldn't the addressed by a variation of the "ignore exception" policy which is meant to just return? A simple extension to log such errors could be crafted from this idea and would make a great example on how to make one's own exception policy. What is missing from this idea?

...

In Christ, Steven Watanabe

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

Antony Polukhin

5:41 p.m.

2017-03-11 20:22 GMT+03:00 Robert Ramey via Boost :

...

On 3/11/17 7:52 AM, Steven Watanabe via Boost wrote:

...
AMDG

On 03/11/2017 02:00 AM, Antony Polukhin via Boost wrote:

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

This also requires an extra function to combine two ExceptionPolicies for binary operators. You'll run into problems with comparison operators, though, as a bool can't hold an ExceptionPolicy.

Couldn't the addressed by a variation of the "ignore exception" policy which is meant to just return?

A simple extension to log such errors could be crafted from this idea and would make a great example on how to make one's own exception policy.

What is missing from this idea?

State of the Policy is missing. In many cases it could be useful to have a Policy with state to be able to get the UB info. This may be useful for different try_* functions that must not throw, usually are in hot path and rarely trigger UB: bool try_do(int x_native) noexcept { bool was_an_error = false; safe x(x_native, &was_an_error); // remembers that error was triggered in the `was_an_error` variable ++x; // ... other operations on `x` // ... // `x` is bad, after comparison `y` is also bad safe y = x; // also copies pointer to `was_an_error` if (x < y) { // ... other operations on `y` } return was_an_error; } Throwing and catching an exception in such function may affect performance. Rewriting the whole function using functions from include/checked.hpp may take a lot of effort and will make the function harder to read. But this is more like a feature request, not a blocker to the library acceptance. looks like they could be added later. -- Best regards, Antony Polukhin

Robert Ramey

6:08 p.m.

On 3/11/17 9:41 AM, Antony Polukhin via Boost wrote:

...

2017-03-11 20:22 GMT+03:00 Robert Ramey via Boost :

...
On 3/11/17 7:52 AM, Steven Watanabe via Boost wrote:

...

...

...
A simple extension to log such errors could be crafted from this idea and would make a great example on how to make one's own exception policy.

What is missing from this idea?

State of the Policy is missing. In many cases it could be useful to have a Policy with state to be able to get the UB info. This may be useful for different try_* functions that must not throw, usually are in hot path and rarely trigger UB:

bool try_do(int x_native) noexcept { bool was_an_error = false; safe x(x_native, &was_an_error); // remembers that error was triggered in the `was_an_error` variable ++x; // ... other operations on `x` // ...

// `x` is bad, after comparison `y` is also bad safe y = x; // also copies pointer to `was_an_error` if (x < y) { // ... other operations on `y` }

return was_an_error; }

template<class call_back> struct exception_logger { // implements ExceptionPolicy concept ... }; what's the matter with the following? void handler(?){ ... // log the error } bool try_do(int x_native) no_except { bool was_an_error = false safe x{x_native}; // handler logs error ++x; // other operations on x ... }

...

Throwing and catching an exception in such function may affect performance. Rewriting the whole function using functions from include/checked.hpp may take a lot of effort and will make the function harder to read.

But this is more like a feature request, not a blocker to the library acceptance. looks like they could be added later.

Right - we're in serious jeopardy of scope creep. Consumer of all ambitious projects. Still, it's becoming clear that some enhancement of the ExceptionPolicy concept might create opportunities with little or no cost in conceptual integrity. We're open to modest specific suggestions. Robert Ramey

Antony Polukhin

7:24 p.m.

2017-03-11 21:08 GMT+03:00 Robert Ramey via Boost :

...

On 3/11/17 9:41 AM, Antony Polukhin via Boost wrote:

...
2017-03-11 20:22 GMT+03:00 Robert Ramey via Boost :

...
On 3/11/17 7:52 AM, Steven Watanabe via Boost wrote:

...
...
...
A simple extension to log such errors could be crafted from this idea and would make a great example on how to make one's own exception policy.

What is missing from this idea?

State of the Policy is missing. In many cases it could be useful to have a Policy with state to be able to get the UB info. This may be useful for different try_* functions that must not throw, usually are in hot path and rarely trigger UB:

bool try_do(int x_native) noexcept { bool was_an_error = false; safe x(x_native, &was_an_error); // remembers that error was triggered in the `was_an_error` variable ++x; // ... other operations on `x` // ...

// `x` is bad, after comparison `y` is also bad safe y = x; // also copies pointer to `was_an_error` if (x < y) { // ... other operations on `y` }

return was_an_error; }

template<class call_back> struct exception_logger { // implements ExceptionPolicy concept ... };

what's the matter with the following?

void handler(?){ ... // log the error }

bool try_do(int x_native) no_except { bool was_an_error = false safe x{x_native}; // handler logs error ++x; // other operations on x ... }

...
Throwing and catching an exception in such function may affect performance. Rewriting the whole function using functions from include/checked.hpp may take a lot of effort and will make the function harder to read.

But this is more like a feature request, not a blocker to the library acceptance. looks like they could be added later.

Right - we're in serious jeopardy of scope creep. Consumer of all ambitious projects.

Still, it's becoming clear that some enhancement of the ExceptionPolicy concept might create opportunities with little or no cost in conceptual integrity. We're open to modest specific suggestions.

Changes to deal with statefull policies are pretty small and could be added any time later: template< class Stored, Stored Min, Stored Max, class P, // promotion polic class E // exception policy > class safe_base : private P, private E /* empty base optimization will make sure that safe_base size does not change for empty/stateless P and E */ { BOOST_CONCEPT_ASSERT((Integer<Stored>)); BOOST_CONCEPT_ASSERT((PromotionPolicy<P>)); BOOST_CONCEPT_ASSERT((ExceptionPolicy<E>)); Stored m_t; // ... template<class T> constexpr /*explicit*/ safe_base(const T & t, P p = {}, E e = {}); /* added P and E as a parameters */ // Added followin 4 functions: const E& exception_policy() const noexcept { return *this; } E& exception_policy() noexcept { return *this; } const P& promotion_policy() const noexcept { return *this; } P& promotion_policy() noexcept { return *this; } // looks like no other changes required }; //////// Now the example becomes the following: struct lazy { bool* was_an_error; void no_error(const char *) const noexcept { *was_an_error = true; } void uninitialized_error(const char *) const noexcept { *was_an_error = true; } void overflow_error(const char *) const noexcept { *was_an_error = true; } void underflow_error(const char *) const noexcept { *was_an_error = true; } void range_error(const char *) const noexcept { *was_an_error = true; } void domain_error(const char *) const noexcept { *was_an_error = true; } bool is_error() const noexcept { return *was_an_error; } }; bool try_do(int x_native) noexcept { bool was_an_error = false; safe x(x_native, {}, lazy{&was_an_error}); // remembers that error was triggered in the `was_an_error` variable ++x; // ... other operations on `x` // ... // `x` is bad, after comparison `y` is also bad safe y = x; // also copies pointer to `was_an_error` if (x < y) { // ... other operations on `y` // // Also works well: return y.exception_policy().is_error(); } return was_an_error; } -- Best regards, Antony Polukhin

John McFarlane

6:13 p.m.

On Sat, Mar 11, 2017 at 9:23 AM Robert Ramey via Boost < boost@lists.boost.org> wrote:

...

On 3/11/17 7:52 AM, Steven Watanabe via Boost wrote:

...
AMDG

On 03/11/2017 02:00 AM, Antony Polukhin via Boost wrote:

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

This also requires an extra function to combine two ExceptionPolicies for binary operators. You'll run into problems with comparison operators, though, as a bool can't hold an ExceptionPolicy.

Couldn't the addressed by a variation of the "ignore exception" policy which is meant to just return?

A simple extension to log such errors could be crafted from this idea and would make a great example on how to make one's own exception policy.

What is missing from this idea?

The solution I favor is for `std::common_type` to be specialized for allowed combinations. For example, `std::common_type_t`, would be `ExceptionPolicy` and `std::common_type_t` would be `T`. Thus, safer types would "win out" over less safe types.

...

...
In Christ, Steven Watanabe

_______________________________________________ Unsubscribe & other changes:

http://lists.boost.org/mailman/listinfo.cgi/boost

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

Oswin Krause

5:51 p.m.

On 2017-03-11 10:00, Antony Polukhin via Boost wrote:

...

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

I would also vote for a class similar like that. However, i would move the whole interface closer to expected in that case. e.g. I might want to know what happened. Lets call this class checked<int>. The behaviour should be like this: all operations between a checked<int> and other integers should work independent of the state of the checked<int> object. However the value of a checked<int> can only be obtained using: checked<int> myInt= ... if(myInt){ int val = myInt.value(); }else if(myInt.error() == overflow){ //some error handling here }else{ //some terminal error handling here } Best, Oswin

Robert Ramey

6:17 p.m.

On 3/11/17 9:51 AM, Oswin Krause via Boost wrote:

...

On 2017-03-11 10:00, Antony Polukhin via Boost wrote:

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

I would also vote for a class similar like that. However, i would move the whole interface closer to expected in that case. e.g. I might want to know what happened. Lets call this class checked<int>.

The behaviour should be like this: all operations between a checked<int> and other integers should work independent of the state of the checked<int> object. However the value of a checked<int> can only be obtained using:

checked<int> myInt= ... if(myInt){ int val = myInt.value(); }else if(myInt.error() == overflow){ //some error handling here }else{ //some terminal error handling here }

cases like this I think would be better handled by using the Checked Integer module of the library directly. int myInt; checked_result<int> val = checked::add(myInt, 42); if(val.exception()) // some eror handling else // ? myInt = val; // convert modadic val back to simple integer. The nature of the a boost library doesn't really promote this but it often happens there there is a sub component of a library which is useful in it's own right. But being a sublibrary - not reviewed independently, makes it sort of invisible to all but the most ardent boost spelunkers. It's a weakness in our system. I don't see a way to address it without make the whole process even more complex. Robert Ramey

Robert Ramey

6:43 p.m.

On 3/11/17 1:00 AM, Antony Polukhin via Boost wrote:

...

Nitpicks:

More generic streaming operators could be profitable (better to use basic_ostream instead of ostream) https://github.com/robertramey/safe_numerics/blob/master/include/interval.hp...

I want to replace ostream with basic_stream but I couldn't figure out the template definition syntax for using two separate sets of template parameters. Someone knows the answer off hand. Please save me some time and post here - after you're sure it compiles.

...

Many operations may be decorated with noexcept

right - I'd be curious to know what happens if one uses noexcept and also uses -fno_exceptions in the command line of some compileers.

...

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

I'm going to remove it.

...

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

Ahhh - no a very sore point. Implicit conversions between numeric type are supported by the library - just as they are for builtin integer types. This is to support the "drop-in" replacability goal. The difference is that these conversions are checked at compile time or execution time. tl;dr; This is actually a very subtle and difficult issue brought up by vicente. I'm going to paraphrase. Aren't we promoting bad practice by encouraging implicit conversions? Or Isn't the way to program correctness paved with strong types? I've a big promoter of this later view. But I confess that making the library has led me to conclude that the future of correct programming needs a more nuanced view. In order to promote correct code, we need to be able to express our ideas directly in code in a way that matches the way we use the ideas in real life. In real life we say x+y with no ambiguity. We can't express real numbers in computer hardware so x+y doesn't really capture what we want to say. requiring explicit conversions every where is going to make it harder to verify that our program matches our expectation. So (checked) implicit conversions are the correct approach. It maintains our ability to visually compare/verify that our code matches our intention - but it traps the times when our expectation can't be actually realized inside the computer hardware. I've become convinced that this is the way things have to be.

...

Those headers are very heavy https://github.com/robertramey/safe_numerics/blob/7ee656ecdf23f6f29f93331769... Try using <iosfwd> and making all the function below to use `basic_ostream&`. right

No need to define those in `namespace std` https://github.com/robertramey/safe_numerics/blob/7ee656ecdf23f6f29f93331769... . Defining those in `boost::numeric` namespace works fine (thanks to ADL!). Hmmm - I didn't know that.

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

will look into it.

...

Robert Ramey

Antony Polukhin

7:02 p.m.

2017-03-11 21:43 GMT+03:00 Robert Ramey via Boost :

...

On 3/11/17 1:00 AM, Antony Polukhin via Boost wrote:

...
Nitpicks:

More generic streaming operators could be profitable (better to use basic_ostream instead of ostream)

https://github.com/robertramey/safe_numerics/blob/master/include/interval.hp...

I want to replace ostream with basic_stream but I couldn't figure out the template definition syntax for using two separate sets of template parameters. Someone knows the answer off hand. Please save me some time and post here - after you're sure it compiles.

An example, that may help you: https://github.com/apolukhin/type_index/blob/develop/include/boost/type_inde... All you have to do, is to list typenames from left to right in the order they appear in function params. Something like this: template std::basic_ostream& operator<<( std::basic_ostream& os, const boost::numeric::interval<T> & i) { os << "[" << i.l << "," << i.u << "]"; return os; }

...

...
Many operations may be decorated with noexcept

right - I'd be curious to know what happens if one uses noexcept and also uses -fno_exceptions in the command line of some compileers.

Works fine on all the compilers I know. To be more sure, you could use BOOST_NOEXCEPT (this will also solve issues with some perverse compilers that have C++14 features without some of the C++11 features like noexcept).

...

...
There's a bunch of implicit conversions in the library

https://github.com/robertramey/safe_numerics/blob/master/include/safe_base.h... (BTW, have not found a body of this one). Would it be better to make them explicit?

Ahhh - no a very sore point. Implicit conversions between numeric type are supported by the library - just as they are for builtin integer types. This is to support the "drop-in" replacability goal. The difference is that these conversions are checked at compile time or execution time.

tl;dr; This is actually a very subtle and difficult issue brought up by vicente. I'm going to paraphrase. Aren't we promoting bad practice by encouraging implicit conversions? Or Isn't the way to program correctness paved with strong types? I've a big promoter of this later view. But I confess that making the library has led me to conclude that the future of correct programming needs a more nuanced view. In order to promote correct code, we need to be able to express our ideas directly in code in a way that matches the way we use the ideas in real life. In real life we say x+y with no ambiguity. We can't express real numbers in computer hardware so x+y doesn't really capture what we want to say. requiring explicit conversions every where is going to make it harder to verify that our program matches our expectation. So (checked) implicit conversions are the correct approach. It maintains our ability to visually compare/verify that our code matches our intention - but it traps the times when our expectation can't be actually realized inside the computer hardware.

I've become convinced that this is the way things have to be.

Two approaches could be mixed: * for promotions and non-narrowing conversions an implicit conversion could be used. * narrowing conversions could be implemented as explicit conversions with runtime check. But that's just an idea that needs some investigation. However that approach is more future proof. If later you decide to make all the conversions implicit - this will not break users code. With current approach - making the conversions explicit will break users code. -- Best regards, Antony Polukhin

2801

Age (days ago)

2801

Last active (days ago)

List overview

Download

10 comments

5 participants

participants (5)

Antony Polukhin
John McFarlane
Oswin Krause
Robert Ramey
Steven Watanabe