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

[Greg Chicares]

On 2017-01-20 14:24, Vadim Zeitlin wrote:
> On Thu, 19 Jan 2017 23:04:02 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> I'm done with
> GC>   
> https://github.com/vadz/lmi/pull/52/commits/d1fd3ae2e222c6d7a9edd1c9124ef2935e2918a5
> GC> and have pushed the final changes.
[...]
>  One small thing I'm still not completely satisfied about (I have a real
> lack of talent for unconditional happiness, as you might have noticed) is

I think of it as a positive talent to call a time-out when something
bothers you.

> this idea that we shouldn't be using "for(auto& foo : foos)" because "foo"
> and "foos" names are too close. IMHO this is the most natural way of naming
> things (supposing that you use something a bit more descriptive instead of
> a literal "foo")

Consider:

---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
diff --git a/msw_workarounds.cpp b/msw_workarounds.cpp
index 3572940..e85678a 100644
--- a/msw_workarounds.cpp
+++ b/msw_workarounds.cpp
@@ -67,11 +67,11 @@ void MswDllPreloader::PreloadDesignatedDlls()
 {
     configurable_settings const& c = configurable_settings::instance();
     std::istringstream iss(c.libraries_to_preload());
-    std::istream_iterator<std::string> i(iss);
+    std::istream_iterator<std::string> library_to_preload(iss);
     std::istream_iterator<std::string> const eos;
-    for(; eos != i; ++i)
+    for(; eos != library_to_preload; ++library_to_preload)
         {
-        PreloadOneDll(*i);
+        PreloadOneDll(*library_to_preload);
         }
     fenv_initialize();
 }
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------

I find "libraries_to_preload" and "library_to_preload" too similar.
And, although that's somewhat contrived, here's a distinction I made
many years ago:

/opt/lmi/free/src/lmi[0]$grep -w GrossPmt *.?pp |wc -l
20
/opt/lmi/free/src/lmi[0]$grep -w GrossPmts *.?pp |wc -l
44

which makes maintenance challenging for me.

> and the extra cognitive burden of having to remember that
> "i" is actually a "foo" is much worse than any, non-existent IMO, as the
> compiler will always catch it, potential for mixing up "foo" and "foos".

Let's take an example from a recent commit. Most of the ranged
for statements are short, so I tried to find one that uses its
iterated value several times:

    for(auto& i : cell_values)
        {
        if(i->PrecedesInforceDuration(year, month))
            {
            continue;
            }
        i->Month = month;
        i->CoordinateCounters();
        i->IncrementBOM(year, month, case_k_factor);
        assets += i->GetSepAcctAssetsInforce();
        }

Suppose I want to know what the last statement:
        assets += i->GetSepAcctAssetsInforce();
does. I can't figure that out without recognizing that we're in a
loop, so I have to search backward for 'for(', which tells me we're
iterating over cell values. Therefore, that statement gets the
current iterand's inforce separate-account assets and accumulates
it into 'assets'--which doesn't depend on 'i', so it's probably the
total for the whole set of cell_values.

Correct me if I'm misleading myself here, because I'm puzzled, but
I guess you might feel that would be clearer if it read:

-        assets += i->GetSepAcctAssetsInforce();
+        assets += current_cell->GetSepAcctAssetsInforce();

Okay, one could extract the modified line from its context, and
it's clearer in isolation because it restates the iterand's meaning.
But is that the advantage you seek--do you really read any single
statement in a loop without looking back to see its context? and
if so...well, I'm puzzled again, because I don't think it can be
understood outside its context. I mean, we could gain a mechanical
understanding of it in isolation, but we can't hope to know what
it really means or why it was written at all.

OTOH, I find the line with s/i/current_cell/ above harder to read:
for one thing, it has eleven more characters. If all the statements
were changed that way, then in order to make sense of them I'd have
to translate 'current_cell' back to my mental concept: it's just the
loop's iterand (so why wasn't it given the obvious name 'i'?). I
can't parse a statement in a loop without determining what the
controlling statement means and, thus, identifying the iterand.
IOW, the compiler might place its address in a register--let's call
it BX--and my mental loop parser has a hard-coded register that's
generally named 'i'. If its physical name is 'i', then I understand
it directly; if it's something else, I have to map it mentally to
'i', and that costs me extra cycles.

And it's really hard to devise good names. As I edit this email
before sending it, I look at this:
  assets += current_cell->GetSepAcctAssetsInforce();
and realize that, which the iterand is "current", it is certainly
not a "cell": IIRC, it's a shared_ptr<AccountValue> that was
constructed from a cell. Whatever name first occurs to me may
very well introduce confusion--unless it's 'i', which takes no
effort to choose.

As I'm sure we've discussed before, I also have a hard-coded
register named 'z', which means whatever we would obviously load
into our fastest register: the thing we're working with. Thus,
grepping for '-w z':

  Country                          = z.Country

Without even looking at the context, I know this is something
like an assignment operator: it assigns our 'Country' member from
the corresponding member of "the most obvious possible thing",
which must be the object assigned from.

  double operator()(double z) {return std::pow(z, 19);}

Here, 'z' is the number we're raising to the 19th power. It's
'z', not 'value' or 'base' or 'number': those names all suggest
connotations that may turn out to be misleading, and they all
require more thought for me to parse than 'z'.

A useful incidental benefit is that I can copy and paste code
written in terms of 'z' between files or functions that use 'z'.
The same goes for 'i'.

> GC> Thanks for providing two separate patches. That was especially
> GC> helpful for 'group_values.cpp': the first patch was easy, but the
> GC> second was not. You may recall that you moved an int variable 'j'
> GC> outside an old-style for statement. Careful study showed that the
> GC> program is defective with that change; but more careful study
> GC> revealed that it was identically defective without the change,
> GC> and has been so since 20050416T0206Z.
> 
>  Sorry about this, I tried to preserve the existing code behaviour without
> worrying too much about whether it was correct or not, maybe I should have
> been more diligent.

Perhaps the first clause of my last quoted sentence was too edgy;
I meant it only as a setup for the self-criticism that follows it.
It is good that we're going through some fine details of this old
code and discovering such problems. It was only because you moved
that 'j' that I was at last able to discern this ancient defect,
and I'm grateful to you for that.