Re: [lmi] [lmi-commits] master b518132 4/4: Remove the latent defect just added

[Greg Chicares]

On 2018-09-07 01:22, Vadim Zeitlin wrote:
> On Fri, 7 Sep 2018 00:45:38 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> I hope you'll find commit ad23b9ee less obtrusive.
> 
>  Now that I do see it, I can say that I absolutely find it a big
> improvement, but, being what I am, still can't prevent myself from thinking
> that it would be even better if we had a safe_denominator() function which
> would really assert in case the precondition is not satisfied because IMO
> this problem should be flagged as an assertion failure, i.e. a programming
> mistake, and not just a run of the mill runtime_error because there is a
> big difference between the former, which has to be fixed, and the latter,
> which is unavoidable and just needs to be handled correctly.
> 
>  So at the very least I'd replace this runtime_error with logic_error. But
> an assertion failure would be even more perfectly suitable here IMHO.
There are at least two distinct matters to discuss here.

(1) Which class derived from std::exception would be best here? I do tend
to use runtime_error in almost every situation, treating runtime vs. logic
errors as "who" vs. "whom" in contemporary American English, where case
distinctions continue to be lost and "who" is always acceptable.

I don't know a workable algorithm for deciding which to use. C++17 (N4659)
says [22.2p2-3]:

| 2 The distinguishing characteristic of logic errors is that they are
|   due to errors in the internal logic of the program. In theory, they
|   are preventable.
| 3 By contrast, runtime errors are due to events beyond the scope of
|   the program. They cannot be easily predicted in advance.

Suppose I write a standalone GUI program for formatting tables, with
an input screen like:

  [spin control] "total rows"
  [spin control] "rows per group"
  [spin control] "max lines per page"

and, following the signature
  paginator::paginator(int total_rows, int rows_per_group, int 
max_lines_per_page)
, I constrain the spin controls to be integers. If someone enters "0"
in the middle one, that would seem to be a logic_error, because I
could have prevented it by constraining the middle one to be positive.

However, further suppose that class paginator is in a shared library,
and the GUI program is a physically distinct client of that library.
What sort of error is zero rows per group, and where? In the GUI
client, it could have been prevented as above, so it's a logic_error
there, except that it doesn't arise there--it's the library that
throws the exception. But the library can't prevent it AFAICS: it's
not an error "in the internal logic" of the library, so it's not a
logic_error there; it must instead be a runtime_error.

Put more tersely: if wxWidgets threw exceptions, and lmi attempts to
instantiate an impossible wxObject of some sort, then which kind of
exception would the library report? I'm thinking that if a library
throws a logic_error, that means there's a defect in the library, but
in this case the library authors cannot even in theory prevent it, so
it can't be a logic error.

(2) If we distinguish two types of exceptions, then why not go a level
deeper into the standard hierarchy and use the seven leaf classes only?
Here, I find the standard less helpful--the likeliest four say only:

| invalid_argument ... to report an invalid argument
| domain_error ... to report domain errors
| out_of_range ... to report an argument value not in its expected range
| range_error ... to report range errors in internal computations

Zero rows per group is...which one? It's an invalid argument, because
valid arguments are positive. It's arguably a domain error, because
the argument's domain is positive integers. That argument is also
not in its expected range. And the immediate problem is division by
zero as a consequence of using this argument in internal computations,
so is it a range_error? Or, more precisely, isn't it domain error in
internal computations, thus partaking of both the domain_error and
the range_error nature, and therefore of both the logic_error and the
runtime_error nature because those are the respective parent classes?

I'm not firmly opposed to making this particular one a logic_error;
but I need a sensible rule that I can figure out how to follow.

>> it would be even better if we had a safe_denominator() function

In the case at hand, I wouldn't use it, because I want to make a
stronger "assertion", namely, that rows_per_group is positive:

    ,rows_per_group_     {0 < rows_per_group ? rows_per_group : throw yikes}
    ,lines_per_group_    {rows_per_group_ + 1}
    ,groups_per_page_    {(max_lines_per_page_ + 1) / lines_per_group_}

Ensuring that the denominator {rows_per_group_ + 1} is nonzero is
a weaker requirement.

As for the general case, consider this code in 'math_functions.hpp':

  template<typename T>
  inline T outward_quotient(T numerator, T denominator)
  {
      static_assert(std::is_integral<T>::value);
      LMI_ASSERT(0 != denominator);

I think that last quoted line is already ideal and perfect, so
I don't think a safe_denominator() function can improve upon it;
thus, I think the only use case for safe_denominator() would be
in mem-initializers, and the only other candidate I find in lmi is:

  template<typename RealType>
  round_to<RealType>::round_to(int decimals, rounding_style a_style)
      :decimals_          {decimals}
      ,style_             {a_style}
      ,scale_fwd_         {detail::perform_pow(max_prec_real(10.0), decimals)}
      ,scale_back_        {max_prec_real(1.0) / scale_fwd_}

where the denominator, returned from this function...

  /// Raise 'r' to the integer power 'n'.
  template<typename RealType>
  RealType perform_pow(RealType r, int n)

...cannot hardly be zero--but I'll add a test like this anyway:

---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
diff --git a/round_to_test.cpp b/round_to_test.cpp
index 4b5b62a3..21973835 100644
--- a/round_to_test.cpp
+++ b/round_to_test.cpp
@@ -543,6 +543,13 @@ int test_main(int, char*[])
     BOOST_TEST(2 == round1.decimals());
     BOOST_TEST(r_to_nearest == round1.style());
 
+    // Try to provoke division by zero in ctor-initializer.
+    BOOST_TEST_THROW
+        (round_to<double>(-1000000, r_to_nearest)
+        ,std::domain_error
+        ,"Invalid number of decimals."
+        );
+
     // The software default rounding style and the hardware rounding
     // mode may be either synchronized or not, so test both ways.
     std::cout << "  Default style synchronized to hardware mode:\n";
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------