Re: [lmi] [PATCH] C++ m11n: range-based for loops

[Greg Chicares]

On 2017-01-18 10:01, Greg Chicares wrote:
> On 2017-01-16 15:43, Greg Chicares wrote:
>> On 2017-01-15 17:59, Greg Chicares wrote:
>> [...]
>>> And here's a freshly-annotated update to the plan I posted earlier,
>>> reflecting the changes I pushed a few minutes ago.
>> 
>> I pushed some more changes; here's a further update.

I'm done with
  
https://github.com/vadz/lmi/pull/52/commits/d1fd3ae2e222c6d7a9edd1c9124ef2935e2918a5
and have pushed the final changes. Below (for the last time) is the
previously-posted plan with final annotations.

In reviewing these changes, I have noticed some opportunities for
further improvements, and found an ancient defect; I'll start
working on those soon.

Thanks for providing two separate patches. That was especially
helpful for 'group_values.cpp': the first patch was easy, but the
second was not. You may recall that you moved an int variable 'j'
outside an old-style for statement. Careful study showed that the
program is defective with that change; but more careful study
revealed that it was identically defective without the change,
and has been so since 20050416T0206Z. The defect is on a code path
that perhaps no end user has ever exercised, but that doesn't mean
that there's no reasonable use for it.

 ------clang-tidy------               ------VZ-manual------
                      -----common------

+authenticity_test.cpp        |  7 ||*authenticity_test.cpp        |  6
+census_view.cpp              | 56 ||+census_view.cpp              | 18
+crc32.cpp                    |  4 ||-crc32.cpp                    |  4
+dbdict.cpp                   | 15 ||*dbdict.cpp                   | 10
+group_values.cpp             | 25 ||+group_values.cpp             | 73
+input_sequence.cpp           | 16 ||*input_sequence.cpp           |  2
+input_sequence_entry.cpp     | 20 ||+input_sequence_entry.cpp     |  6
+ledger.cpp                   | 18 ||+ledger.cpp                   | 20
+wx_table_generator.cpp       | 18 ||*wx_table_generator.cpp       |  8

                      -----unique------

                                     -actuarial_table.cpp          |  4
+any_member.hpp               | 19
                                     +authenticity.cpp             |  4
                                     -bcc_ld.cpp                   | 11
                                     *ce_product_name.cpp          |  8
                                     *ce_skin_name.cpp             |  8
                                     +database_view.cpp            |  7
+dbnames.cpp                  |  5
+gpt_input.cpp                |  9
                                     +gpt_state.cpp                |  5
+group_quote_pdf_gen_wx.cpp   | 27
+icon_monger.cpp              |  3
+ihs_acctval.cpp              |  9
                                     +illustrator.cpp              | 18
+input_harmonization.cpp      | 27
+input_realization.cpp        | 15
                                     +input_seq_helpers.hpp        | 10
+ledger_base.cpp              | 84
+ledger_text_formats.cpp      | 39
+ledger_xml_io.cpp            | 18
+main_cli.cpp                 |  6
+main_wx_test.cpp             | 21
+mec_input.cpp                | 10
+miscellany.cpp               |  4
+multiple_cell_document.cpp   | 37
+mvc_controller.cpp           | 56
+mvc_model.cpp                |  7
+policy_document.cpp          | 18
+policy_view.cpp              | 32
+preferences_model.cpp        | 27
-print_matrix.hpp             |  4
+product_data.cpp             |  5
+regex_test.cpp               | 10
+rounding_document.cpp        | 10
+rounding_view.cpp            | 23
+single_cell_document.cpp     |  9
+skeleton.cpp                 | 24
+test_coding_rules.cpp        |  8
+tier_document.cpp            |  6
+tier_view.cpp                |  4
+view_ex.tpp                  |  4
+wx_test_about_version.cpp    | 25
                                     *wx_test_benchmark_census.cpp |  6
+wx_test_input_sequences.cpp  |  4
+wx_test_input_validation.cpp |  4
+wx_test_paste_census.cpp     |  9
+wx_utility.cpp               |  4
+xml_lmi.cpp                  |  9
+xml_serializable.tpp         | 10
+xml_serialize.hpp            | 11

  49 files changed                 || 19 files changed
 310 insertions                    || 110 insertions
 525 deletions                     || 118 deletions

-------------------------------------------------------------------------------
* Removed directory_iterator changes, which means
    no changes at all to these files:
      ce_product_name.cpp ce_skin_name.cpp wx_test_benchmark_census.cpp
    only clang-tidy changes to this file:
      dbdict.cpp
  Similarly marked files where the second ("manual") patch changed only
  whitespace or variable names:
      authenticity_test.cpp input_sequence.cpp wx_table_generator.cpp

+ Applied; comments, if any, below.

+wx_test_paste_census.cpp
We should certainly dispense with the iterator typedef. But compare
your patch to what I committed: do you still want to introduce an
extra variable? Compare the change in
  group_quote_pdf_generator_wx::output_document_header()
where we kept the iterator precisely so we could test it against end().

- Not applied, as explained below.

-actuarial_table.cpp
I think this is clearer as classic C. Data insertion is driven by the
number of values available, not the vector's size. Calling
    data_.resize(number_of_values);
was just a performance optimization.

-bcc_ld.cpp
Unlike
  http://lists.nongnu.org/archive/html/lmi/2017-01/msg00092.html
I'm maintaining it, so I'll leave it the way I prefer.

-crc32.cpp
Second patch not applied. Reason: I think this function is easier to
read if all three for loops in this function are left as classic C,
instead of changing one of them to the C++11 dialect.

-print_matrix.hpp
Nested for loops are clearer if all use the same dialect.