Re: [boost] [safe_numerics] Review Part 1 (Documentation)

Subject: Re: [boost] [safe_numerics] Review Part 1 (Documentation)
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2017-03-02 20:12:47

AMDG

All comments are as of 3bcfabe1cafb8ef6edf08d3bd8730a1555b1c45d

General notes:

You should have a root index.html that redirects
to the top level documentation page.

There are two copies of proposal.pdf in the
repository.

safe_numerics.pdf seems to be out-dated.

VS2015: FAIL (as expected)

directory structure is not quite boost ready.
(needs include/boost/numeric/ instead of just include/)

The include paths used in the documentation are a mess.
I see at least 6 different variations:
- boost/safe_numerics/include/
- boost/safe_numerics/
- safe/numeric/
- boost/numeric/
- safe_numerics/include/
- ../include
- ../../include

In boostbook, <code language="c++"> enables C++
syntax highlighting for code snippets.  (This is
enabled by default for programlisting, but <code>
is used for non-C++ content enough that it needs
to be specified.)

Introduction:

"Since the problems and their solution are similar, We'll..."
"We" should not be capitalized.

"...the results of these operations are guaranteed to be
either arithmetically correct or invoke an error"
"either ... or ..." here has a noun on one side and
a verb on the other.  Try "... either to be ... or to invoke ..."

"#include <boost/safe_numeric/safe_integer.hpp>"
I believe that this should be "safe_numerics"
(with an "s")

"Enforce of other program requirements using ranged integer types."
Delete "of"

"The library includes the types safe_range(Min, Max) and safe_literal(N)"
These don't look like any C++ type I've ever seen unless
they're macros.

tutorial/1.html

"When this occurs, Most"
lowercase "Most"

"When this occurs, Most C++ compilers will continue to execute with no
indication that the results are wrong"
Is it the compiler that continues to execute or is it the
program built by the compiler?

"        // converts variables to int before evaluating he expression!"
s/he/the/

"The solution is to replace instances of char type with safe<char> type"
There are no char's in this example.

tutorial/2.html:

"This is a common problem with for loops"
Is this really true?  The most common pattern
of integer for loops is for(i=0;i<n;++i)
which can only overflow when n is a wider
type than i.  I usually see this with int/size_t,
but that will get a signed/unsigned comparison
warning on some compilers.

tutorial/5.html:

Creating the bounds type is also somewhat error-prone.
It would be quite easy for C++ programmers who are used
to half-open ranges to use safe_unsigned_range<0, i_array.size()>

tutorial/7.html
        std::cout << x / y;
        std::cout << " error detected!" << std::endl;
Should this be "NOT detected"?

eliminate_runtime_penalty.html:

The name trap_exception doesn't convey the meaning
of causing a compile-time error to me.  To me the term
'trap' implies a signal handler.

eliminate_runtime_penalty/1.html:

The links for native, automatic, and trap_exception are broken.

eliminate_runtime_penalty/3.html
    throw_exception // these variables need to
need to what?

integer.html:

"std::numeric_limits<T>.is_integer"
should be "::is_integer"

safe_numeric_concept.html:

"However, it would be very surprising if any implementation were to be
more complex that O(0);"
I think you mean O(1).  O(0) means that it takes no time at all.

"... all C++ operations permitted on it's base type..."
s/it's/its/

"( throw exception, compile time trap, etc..)"
Delete the leading space.

promotion_policy.html:

"auto sz = sx + y; // promotes expression type to a safe<long int> which
requires no result checking
is guaranteed not to overflow."
The line wrapping ends up uncommented.

What happens for binary operators when the arguments
have different promotion policies?  Is it an error?
Does the implementation randomly pick one?  Is there
some kind of precedence rule for promotion policies?
(Note: the same thing applies to exception policies)

exception_policy.html:

"EP | A type that full fills the requirements of an ExceptionPollicy"
s/full fills/fulfills/, s/ExceptionPollicy/ExceptionPolicy/

"EP::underflow_error(const char * message)"
Underflow is a floating point error that happens
when the value is too close to 0.  (INT_MIN - 1)
is an overflow error, not an underflow error.

"template<void (*F)(const char *), void (*G)(const char *), void
(*H)(const char *)>
boost::numeric::no_exception_support"
What do the arguments mean?  According to the table,
there a 6 functions, not 3.

safe.html:

"T | Underlying type from which a safe type is being derived"
"derived" has a specific meaning in C++ which is not what you mean.

"...at runtime if any of several events result in an incorrect
arithmetic type."
It isn't the type that's incorrect, is it?

"If one wants to switch between save and built-in types ..."
s/save/safe/

"this type of code will have to be fixed so that implicit
conversions to built-in types do not occur"
So are explicit casts allowed?

"This is because there is no way to guarantee that the expression i * i"
The second 'i' seems to be outside the <code> block.

"This can be justified by the observe"
This sentence is incomplete.

"This can be done by specifying a non-default type promotion policy
automatic"
Should be "policy, automatic."  Also, automatic should probably be <code>

"All the work is done at compile time - checking for exceptions
necessary (input is of course an exception)"
I can't parse this sentence.  Also, the '-' should
be an em-dash, I think.

safe_range.html:

"MIN | must be non-integer literal"
I think this is supposed to be "non-negative integer"
Also, this is only true for safe_unsigned_range.
It would by weird if safe_signed_range didn't
accept negative arguments.

#include <safe/numeric/safe_range.hpp>
The directory safe/numeric/ doesn't exist.

    safe_unsigned_range<7, 24> i
missing ';'

static_assert(std::is_same<decltype(k), safe<unsigned int>);
Syntax error: expected '>' befor ')'

Also, safe is defined in safe_integer.hpp which is not #included.

promotion_policies/native.html:
"It's main function is to trap incorrect..."
s/It's/Its/

promotion_policies/cpp.html

"...we can force the evaluation of C++ expressions test environment..."
"...expressions /in the/ test..."

Unless I'm misunderstanding something, the use
of trap_exception will fail to compile at:
d *= velocity;

"Note the usage of the compile time trap policy in order to
detect at compile time any possible error conditions. As I
write this, this is still being refined. Hopefully this will
be available by the time you read this."
This comment seems out-dated.

exception_policies.html:

"no_exception_support<O, U = O, R =O, D = O>"
So now we have 4 parameters?

exception_policies/trap_exception.html:

"This exception policy will trap at compile time any
operation COULD result in a runtime exception"
There's a missing word here.  "...compile time /if/ any..."
is one possible fix.

exception_policies/no_exception_support.html

Template Parameters table: waaaaaay too many O's.
Also, the first two parameters (no exception and
uninitialized) are missing.

library_implementation.html:

"correct bugs in it, or understand it's limitations"
s/it's/its/

interval.html:

boost/numeric/interval.hpp and boost::numeric::interval
are already taken by Boost.Interval.

checked_result.html:

"checked_result(e, msg)"
'e' was not defined.

"static_cast<const char *>(c) | R | extract wrapped value - asserts if
not possible"
I think this is supposed to return the message as 'const char *'
not the value as 'R'.

exception_type.html:

"dispatch(overflow_error, "operation resulted in overflow");"
The template parameter EP is missing.

rationale.html:

"2. Can safe types be used as drop-in replacement for built-in types?"
replacements (plural)

"#include  <cstdint>
typedef safe_t<std::int16_t> int16_t;"
This may or may not compile as it is unspecified whether
cstdint defines ::int16_t.

In Christ,
Steven Watanabe