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