Re: [boost] [safe_numerics] Formal review starts today

Subject: Re: [boost] [safe_numerics] Formal review starts today
From: Paul A. Bristow (pbristow_at_[hidden])
Date: 2017-03-09 15:23:29

> -----Original Message-----
> From: Boost [mailto:boost-bounces_at_[hidden]] On Behalf Of Andrzej Krzemienski via Boost
> Sent: 01 March 2017 23:37
> To: boost-announce_at_[hidden]; boost_at_[hidden]
> Cc: Andrzej Krzemienski
> Subject: [boost] [safe_numerics] Formal review starts today
> 
> Hi Everyone,
> Today, on March 2nd (or tomorrow, depending on your timezone), is the start
> of the formal review of safe_numerics library by Robert Ramey. The review
> will run till March 11th. I will be serving as review manager.
> 
> safe_numerics is a set of drop-in replacements for the built-in integer
> types, which make the guarantee that the results of operations are either
> correct according to the rules of integer arithmetic or report a
> diagnosable error (rather than offering modulo arithmetic results, or
> resulting in undefined behavior, or returning incorrect results as in the
> case of signed vs. unsigned comparisons).
> 
> The library can be found on GitHub:
> https://github.com/robertramey/safe_numerics/
> 
> Online docs:
> http://htmlpreview.github.io/?https://github.com/robertramey/safe_numerics/
> master/doc/html/index.html
> 
> Formal Review comments can be posted publicly on the mailing list or Boost
> Library Incubator <http://blincubator.com>, or sent privately to the review
> manager, that is myself.
> 
> Here are some questions you might want to answer in your review:
> 
>    - What is your evaluation of the design?

Complicated to use (compared to int), and very, very complicated to write.

But that is a consequence of the daft design of the C language.

The language also doesn't allow use of hardware to handle overflow (and underflow) and doesn't have arrays etc as first class citizens.

I don't believe that it is really a idiot-proof drop-in replacement for the built-in integral types, but it will be fine to be used for new projects, especially as it needed the fancy features of C++14.

I agree that explicit conversions are the Right Thing, but they do carry a cost to the users - the need to understand the issues and take care with construction and assignment because this is a User Defined Type (UDT) and the special-case privileges of built-in do not apply (another daft design decision).  Floating-point and fixed-point UDT have proved unintuitive to use because of explicit conversions; there are big elephant traps awaiting the unwary.

Costly at compile time because of the number of libraries included, but that's a cost worth paying.

I like that the infrastructure might be extended to other than integral types.

>    - What is your evaluation of the implementation?

Above my pay grade.

>    - What is your evaluation of the documentation?

Good exposition of the problems and solutions.

Good examples.

Links to source of examples code would be *very* useful.  Starting with an example is the most common way of 'getting started'.

Will using "" file specification
#include "../include/safe_range.hpp"
instead of <>
#include <../include/safe_range.hpp>
cause trouble for users trying to use/modify the examples in Boost or their own folder position?

The common need for overflow (or underflow) to become 'saturation' == max_value (or min_value) is not an option (but then it is arithmetically 'wrong', I suppose ;-))

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

Very valuable to make programs that 'always work' instead of 'mostly work'.

Users might appreciate a list of compiler versions that really do work. 
Sadly 'Compliant C++14' is a bit fuzzy.  (Should become clearer if accepted and test matrix visible).

A good effort at working round fundamental C language defects.

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

Had a very brief try with VS 2015 with /std:c++latest added to command line (to try to ensure C++14 compliance) but am stuck with

1>j:\cpp\safe_numeric\safe_numeric\include\interval.hpp(107): error C2737: 'boost::numeric::`anonymous-namespace'::failed_result': 'constexpr' object must be initialized
1>j:\cpp\safe_numeric\safe_numeric\include\safe_base_operations.hpp(131): error C2244: 'boost::numeric::safe_base<T,Min,Max,P,E>::operator =': unable to match function definition to an existing declaration

But I am sure that this is entirely my fault, but I'm out of my time allotted to this review.

Also - Is this warning(s) avoidable/relevant/quietable?

j:\cpp\safe_numeric\safe_numeric\include\safe_base.hpp(233): warning C4814: 'boost::numeric::safe_base<T,Min,Max,P,E>::operator =': in C++14 'constexpr' will not imply 'const'; consider explicitly specifying 'const'

I feel that all warnings should be avoided or supressed using push'n'pop pragmas.

>    - How much effort did you put into your evaluation? A glance?

 A quick  reading.

>    - Are you especially knowledgeable about the problem domain?

No.

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

Yes.

Paul

PS I noted a few typos in documentation (though I doubt these have escaped Steven's fine-tooth-combing ;-).

multiplication etc. . C/C++ often automatically  - extraneous .

similar, We'll  - should be lower case w.

the expression will yield the correct mathematical result - missing .

trap at compiler time all operations which might fail at runtime.  - compile-time

(Better to use compile-time and run-time throughout? Inventing new words is unnecessary and hyphenating is good for readbility)

This solution is simple, Just replace instances    - lower case j

parameters are guarenteed to meet requirements when - guaranteed

our program compiles without error - even when using the trap_exception exception policy  - missing .

We only needed to change two lines of code to achieve our goal missing .

( throw exception, compile time trap, etc..) no implementation should return an arithmetically incorrect result.  - extraneous space after ( and missing . and new sentence No implementation ...

C++ operations permitted on it's base type - should be its  (possessive!)
---
Paul A. Bristow
Prizet Farmhouse
Kendal UK LA8 8AB
[phone]