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

Subject: Re: [boost] [safe_numerics] Review Part 1 (Documentation)
From: Robert Ramey (ramey_at_[hidden])
Date: 2017-03-02 22:47:57

On 3/2/17 12:12 PM, Steven Watanabe via Boost wrote:
> AMDG
>
> All comments are as of 3bcfabe1cafb8ef6edf08d3bd8730a1555b1c45d

Very, very helpful.  I'm amazed that I overlook this stuff myself
and that you pick up on it so easily.

The observations that I have not commented on I consider "no brainers"
and I will just fix as recommended.

My inclination is to not fix these things right now.  I don't want to
present reviewers with a moving target as I think it can confuse
things a lot.  I'm thinking that when I get most of the input
I'm going to get, I'll make one final version which incorporates
all the non-controversial changes.

Thanks again for your invaluable observations.

Robert Ramey

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

Hmmm- I wasn't aware that these are in the repository - I'll remove them
>
> VS2015: FAIL (as expected)
>
> directory structure is not quite boost ready.
> (needs include/boost/numeric/ instead of just include/)

I know.  but this is not a requirement for review.
>
> 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

Hmmm - where - in the documentation? In the examples? or?.

I've been dis-inclined to tightly couple this to boost unless it's accepted.

>
> 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.)
Hmmm - right now I'm using program listing and it looks good - syntax is 
colorized.  For inline I'm using <code ...> without the language.  Are 
you suggesting that I change the inline to <code language="c++" ... ?

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

Right - this was a sore point.  I can't make them types.  I think I've 
experimented with making them macros but I'm entirely satisfied with the 
either.  It's a point which needs to be refined.

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

I meant the compiled program.  But now I'm coming upon cases where the 
compiler traps in an invalid constexpr expression.

>
> "        // 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?
LOL - I can't prove it's a common problem with for loops.
 > 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,
right
> but that will get a signed/unsigned comparison
> warning on some compilers.
I've been surprised at how often compilers don't flag this.  Maybe I 
have to include some more switches.
>
> 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.

Ahh names - Boost's favorite subject. I think I use "trap"
to indicate compile time detections: static_assert, errors detected
in constexpr expressions at compile time, etc.  I wanted to distinguish
these from runtime exceptions, assert (which I don't think I use) and
other runtime phenomena.  Signals hasn't come up.  I still think
this is a good convention - at least in this context.

>
> 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.
Hmm - to me O(0) is fixed time while O(1) is time proportional to x^1.
But I'll rephrase the O() notation which is confusing in this context.

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

In both cases:
        at least one policy must be non void
        if both are non void they must be equal

I'll clarify this.

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

Right.  There is no underflow error thrown in the current code.  When I 
made this I had the hope that safe_float would be supported by this 
library and I still have that hope someday.  So I specified it here.

There are other questions besides.  Suppose one has a type "rational 
number" represented by a pair of integers.  I would hope that a 
"rational number" would fulfill the type requirements of this library 
ant might be usable as a T for safe<T>.  But underflow would be 
applicable here.  Of course this opens up all kinds of issues which
I don't think we want to explore here

But for these reasons, I left it in even though the current 
implementation has no reason to use it.

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

Right - will fix.  The code in exception_policies has the correct
number of arguments.
>
> 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.
hmm - OK - I can change that to "Underlying type which a safe type is 
based".  This will work better with the code which uses safe_base for 
some trait or other.
>
> "...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?
They are.  the casting operator is overloaded to guarantee that
either the value is preserved or an error is invoked.

safe<int> i = 1024;
static_cast<char>(i); // will throw exception

constexpr safe<int> i = 1024;
constexpr j = static_cast<char>(i) // will trap a compile time

also implicit casts between safe types are permitted - but they
are checked for errors.

The above is inspired with the goal of achieving drop-in
replacability.

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

this is included by safe_range.hpp.  But I agree that if I use
safe<..> I should include safe_integer.hpp even though it's
redundent.

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

Hmmm - why do you think that?

This is an example take from a real project - (for the tiny PIC
processor).  I didn't make clear but LEMPARAMETER is a typedef
for a 16 bit integer.  So this should never trap...

Damn - I see you're correct here.  Now if I had written

d = velocity * velocity;

I would be golden!

Nope, then there is the possibility that the d can't fit into a uint16
so it could still be wrong.  I guess this illustrates the impossibility
for normal people to actually write demonstrably correct code.

I'm thinking the example should look more like:

// return value in steps
/*
Use the formula:
     stopping dist = v **2 / a / 2
*/
uint16 get_stopping_distance(LEMPARAMETER velocity){
     return velocity * velocity / lem.acceleration / 2;
}

That should be provable correct !!!

Damn - the correct return of uint16 can't be guaranteed at compile
unless we do:

constexpr uint16 get_stopping_distance(LEMPARAMETER velocity){
     return velocity * velocity / lem.acceleration / 2;
}

and only call with constexpr argument.

Ahhh - finally I see your point.  assignment using d as
an accumuator loses the range calculated at compile time
so subsequent operations can't be guaranteed to not
overflow.

I'll expand discussion of this example.

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

Right

I looked at the boost.interval library to see what I might
be able to use.  Looks to me like an underrated gem.  but it
only address floating point and doesn't (of course) support
constexpr.  I don't know if it would have any role in some
future safe_float.

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

I see this is out of whack.

check_result<R> returns a union (or monad if you must) which contains
either the result of type R or an instance of "exception_type" described
in the next section.  Will address.

>
> 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
>
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
>