Re: [lmi] New rate-table implementation

[Vadim Zeitlin]

On Thu, 19 May 2016 14:27:19 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2016-05-19 13:33, Vadim Zeitlin wrote:
GC> > On Thu, 19 May 2016 00:40:24 +0000 Greg Chicares <address@hidden> wrote:
GC> > 
GC> > GC> On 2016-03-19 19:43, Greg Chicares wrote:
...
GC> > So this replacement hasn't been even started yet, let alone
GC> > achieved candidate status. The question is: should I start it now?
GC> 
GC> I don't think so. I'd like to read it first.

 Please let me know when/if I can start working on replacing the existing
code, I've probably already forgotten most of this rate tables stuff by
now, but I still have some vague memories about how it works and it would
be more efficient to put them to use before they completely evaporate.

GC> > GC> I made some superficial changes before committing:
GC> > GC>  - smite files with BDFL rod of renaming +/-7 (sign depends on your 
POV)
GC> >                                                                         
^^^
GC> >  Didn't you mean "alignment"?
GC> 
GC> Indeed. Positive sign if you're lawful chaotic like me.

 I'm neutral, of course.

GC> >  BTW, you may want to use C++11 raw string for regexes, not needing to
GC> > escape backslashes in regexes is really helpful.
GC> 
GC> I resisted the temptation to delay the patch in order to make that
GC> desirable change, especially because I might have introduced a defect
GC> through misunderstanding. It would be nice to use raw strings elsewhere,
GC> too, e.g.:
GC>     oss << "Copyright \\(C\\)[^\\n]*" << year;
GC>     static boost::regex const 
forbidden("[\\x00-\\x08\\x0e-\\x1f\\x7f-\\x9f]");

 Should I make this the next step of C++11 m11n? There is even a way to do
it automatically in the latest (yet unreleased but Debian Sid does already
have it) clang:

http://clang.llvm.org/extra/clang-tidy/checks/modernize-raw-string-literal.html

Although perhaps this would be simple enough to do (semi)manually...

GC> I've been moving gradually to a rule that I'll state by visual example:
GC> 
GC> 00000000011111111112222222222333333333344
GC> 12345678901234567890123456789012345678901
GC> #include "wherever/whatever"            // whatever::frobnicate()
GC> #include <whatever>                     // std::frobnicate()

 Thanks, I've recorded this in my (depressingly empty) lmi/style file. I
wish I could formalize lmi formatting conventions enough to make it
possible to use clang-format with it. Looking at

        http://clang.llvm.org/docs/ClangFormatStyleOptions.html

it seems like it supports many of the particularities of the lmi style,
but I'm not sure about this one: neither AlignTrailingComments nor
SpacesBeforeTrailingComments seem to be appropriate... I'll try
experimenting with it to see if I can achieve the desired result.


GC> > GC> I clumsily revised one use of the stdout redirector because it seemed
GC> > GC> to gobble a useful error message.
GC> > 
GC> >  Sorry, I don't see it, which one?
GC> 
GC> This one, which used the redirector in your original:
GC> 
GC> void test_from_text()
GC> {
GC>     // Using unknown header in a place where it can't be parsed as a
GC>     // continuation of the previous line should fail.
GC>     {
GC>     std::cout << "Expect 'Possibly unknown field...':" << std::endl;
GC>     BOOST_TEST_THROW
GC>         (table::read_from_text("Bloordyblop: yes\n" + simple_table_text)
GC>         ,std::runtime_error
GC>         ,lmi_test::what_regex("expected a field name")
GC>         );
GC>     }
GC> 
GC> I conjecture that you'll see the same problem I saw if you change it thus:
GC> -        ,lmi_test::what_regex("expected a field name")
GC> +        ,"THIS DOES NOT MATCH"

 Indeed, thanks, I see it now. Unfortunately I still have to disagree with
your change. IMNSHO the problem is squarely in test_main.cpp which,
completely unexpectedly, sends test failure reports to stdout instead of
stderr. I don't see any possible rationale for this... With the following
trivial changes:
---------------------------------- >8 --------------------------------------
diff --git a/test_main.cpp b/test_main.cpp
index 81ebc19..9d1cc8a 100644
--- a/test_main.cpp
+++ b/test_main.cpp
@@ -91,7 +91,7 @@ namespace lmi_test
 
     std::ostream& error_stream()
     {
-        return std::cout << "\n**** test failed: ";
+        return std::cerr << "\n**** test failed: ";
     }
 
     void record_error()
@@ -156,7 +156,7 @@ int cpp_main(int argc, char* argv[])
 
     catch(lmi_test::test::test_tools_exception const&)
         {
-        std::cout << "\n**** previous test error is fatal" << std::endl;
+        std::cerr << "\n**** previous test error is fatal" << std::endl;
         // Reset so we don't get two messages.
         lmi_test::test::test_tools_errors = 0;
         result = lmi_test::exit_test_failure;
@@ -164,7 +164,7 @@ int cpp_main(int argc, char* argv[])
 
     if(lmi_test::test::test_tools_errors)
         {
-        std::cout
+        std::cerr
             << "\n**** "
             << lmi_test::test::test_tools_errors
             << " test errors detected; "
diff --git a/catch_exceptions.hpp b/catch_exceptions.hpp
index d4b5c9a..5e17ce2 100644
--- a/catch_exceptions.hpp
+++ b/catch_exceptions.hpp
@@ -169,7 +169,7 @@ namespace lmi_test
                 << std::endl
                 ;
             err
-                << "**********  errors detected; see stdout for details  
***********"
+                << "**********  errors detected; see stderr for details  
***********"
                 << std::endl
                 ;
             }
diff --git a/rate_table_test.cpp b/rate_table_test.cpp
index dd57dfe..c09ed91 100644
--- a/rate_table_test.cpp
+++ b/rate_table_test.cpp
@@ -340,7 +340,7 @@ void test_from_text()
     // Using unknown header in a place where it can't be parsed as a
     // continuation of the previous line should fail.
     {
-    std::cout << "Expect 'Possibly unknown field...':" << std::endl;
+    std_out_redirector std_out_redir;
     BOOST_TEST_THROW
         (table::read_from_text("Bloordyblop: yes\n" + simple_table_text)
         ,std::runtime_error
---------------------------------- >8 --------------------------------------

 I get perfectly informative output in case of test failure:
---------------------------------- >8 --------------------------------------
% ./rate_table_test.exe -a

**** test failed: Caught exception
    'Error reading table from string: expected a field name followed by a colon 
at line 1.
[file rate_table.cpp, line 2318]
'
  when
    'expected a strawberry'
  was expected.
[file rate_table_test.cpp, line 348]

**** 1 test errors detected; 1607 tests succeeded

**** returning with error code 201
**********  errors detected; see stderr for details  ***********
---------------------------------- >8 --------------------------------------

 I hope you can apply the above because the simple output like
---------------------------------- >8 --------------------------------------
.... 1608 tests succeeded
no errors detected
---------------------------------- >8 --------------------------------------
looks much better to me than the noise generated by the current code.

 Sorry for insisting so much on the testing infrastructure, but after
working with it relatively a lot recently and also working with the unit
tests in autotools-based build it's just impossible to ignore how
inconvenient and inefficient lmi tests output is. I know that you don't
consider it to be a problem but I am convinced that it pretty much is one
and it doesn't cost much to fix, or at least strongly improve, it. Could we
please do it?

 Thanks in advance,
VZ