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

[Greg Chicares]

On 2017-01-15 15:28, Vadim Zeitlin wrote:
> On Sun, 15 Jan 2017 03:32:38 +0000 Greg Chicares <address@hidden> wrote:

[...duration_mode_choice_values[] in 'input_sequence_entry.cpp'...]

> GC> The array should be const, of course.
> 
>  Yes, no idea why it isn't already.

> GC> -choice_value duration_mode_choice_values[] =
> GC> +static choice_value const duration_mode_choice_values[] =
[...]
>  But this chunk shouldn't be applied because all this is already inside an
> anonymous namespace and hence this variable already has static linkage.

Okay, thanks, I've added "const" but not "static".

> GC>  void DurationModeChoice::value(duration_mode x)
> GC>  {
> GC> -    for(unsigned int i = 0; i < duration_mode_choices; ++i)
> GC> +    for(auto const& i : duration_mode_choice_values)
[...]
>  The only other thing I might change here would be the name of the loop
> variable as "i" evokes either an index (thanks, Fortran) or an iterator
> while this one is neither and so should rather be called something like
> "choice_value" IMHO.

There's no standard term: I think of it as a "not-an-iterator", or a
"dereferenced iterator", or a "traverser". But it does model the Iterator
design pattern, and therefore I still prefer names like 'i' and 'j'.

And here's a freshly-annotated update to the plan I posted earlier,
reflecting the changes I pushed a few minutes ago. I've gotten through
all but two of the files that were affected by both patches. I might
put those two aside for now (because they're pretty complex) and work
on all the rest (not shown here because I haven't touched them yet).

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

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

- Not applied, as explained below.

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