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

[Vadim Zeitlin]

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

GC> On 2017-01-11 22:39, Vadim Zeitlin wrote:
GC> > 
GC> >           https://github.com/vadz/lmi/pull/52
GC> 
GC> Do you see any reason why I should not apply the following patch in
GC> addition?

 After looking at it more carefully, no, I don't.

GC> The array should be const, of course.

 Yes, no idea why it isn't already.

GC> And the for loop is almost identical to the one in DurationModeChoice's
GC> ctor a few lines above. SetStringSelection() is probably slower than
GC> SetSelection(), but I doubt that makes a noticeable difference.

 I think that when looking at this loop while working on the original patch
I left it alone because I just didn't think about using SetStringSelection()
here, and so decided to keep the index-based loop (which is IMHO preferable
if an index is needed anyhow). But I do agree with you that the overhead of
using SetStringSelection() for a control containing 5 elements should be
completely negligible and the loop looks better without indices.

GC> 
---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
GC> --- a00/52b/input_sequence_entry.cpp        2017-01-14 23:36:42.329382936 
+0000
GC> +++ ./input_sequence_entry.cpp      2017-01-14 23:47:08.030305282 +0000
GC> @@ -75,7 +75,7 @@
GC>      char const*   label;
GC>  };
GC>  
GC> -choice_value duration_mode_choice_values[] =
GC> +static choice_value const duration_mode_choice_values[] =
GC>    {
GC>      {e_retirement,       "until retirement"},
GC>      {e_attained_age,     "until age"},

 But this chunk shouldn't be applied because all this is already inside an
anonymous namespace and hence this variable already has static linkage.

GC> @@ -137,11 +137,11 @@
GC>  
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)
GC>          {
GC> -        if(x == duration_mode_choice_values[i].mode)
GC> +        if(x == i.mode)
GC>              {
GC> -            SetSelection(i);
GC> +            SetStringSelection(i.label);
GC>              return;
GC>              }
GC>          }
GC> 
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------

 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.

 Thanks again for looking at this!
VZ