Re: [lmi] Request for review

[Greg Chicares]

On 2017-01-12 11:12, Greg Chicares wrote:
> On 2017-01-12 10:14, Greg Chicares wrote:
> [...]
>> Rule 1:
>> - for(std::vector<T>::iterator ...
>> + for(T& ...
>> - for(std::vector<T>::const_iterator ...
>> + for(T ...         <-- if T is a builtin type
>> + for(T const& ...  <-- otherwise
> [...]
>> Rule 2:
>> - for(std::vector<T>::iterator ...
>> + for(auto& ...
>> - for(std::vector<T>::const_iterator ...
>> + for(auto const& ...
> [...]
>> Rule 3 (not recommended):
>> - for(std::vector<T>::const_iterator ...
>> + for(auto ...         <-- if T is a builtin type
> 
> Also consider...
> 
> Rule 4: Always write ranged-for loops as 'for(auto& ...'. Rationale:
> 
> http://www.codesynthesis.com/~boris/blog/2012/05/16/cxx11-range-based-for-loop/
> |
> | std::vector<int> v = {1, 2, 3, 5, 7, 11};
> | const std::vector<int> cv = {1, 2, 3, 5, 7, 11};
[...]
> | for (auto& x: v) // x is int&
> | for (auto& x: cv) // x is const int&
> 
> Here's the real point: the compiler deduces 'const'. It costs typing and
> thought to specify 'const', so we should never write 'const' because it
> has no benefit. Except...
> 
> Counterargument: sometimes we really do want to treat a container as
> const even if we could modify it.
[...]
> IOW, writing the const in 'auto const&' says this is a nonmutating loop,
> in much the same way that writing const at the end of member function
> says it's not a mutator. Conclusion: Rule 2 is better than Rule 4.

Counter-counterargument: Lavavej has proposed
  http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n3853.htm
to default the for-range-declaration to 'auto &&'. He has considered
constness; if I want to disagree with such a well-thought-out proposal,
then I must marshal facts, if I can.

About three-quarters of lmi's container iterators are const (if we
rather casually take these 'grep' commands as representative):

/opt/lmi/free/src/lmi[0]$grep '::const_iterator' *.?pp | wc -l 
159
/opt/lmi/free/src/lmi[0]$grep '::iterator' *.?pp | wc -l 
51

Few files have many. I don't want to spend time analyzing them all,
so I'll consider 'mvc_controller.cpp' only. Some of the containers
are const anyway, e.g.:

    std::vector<std::string> const& mn = model_.Names();
    typedef std::vector<std::string>::const_iterator svci;

In this example, we deliberately form a const_iterator to a non-const
container:

    typedef std::vector<std::string>::const_iterator svci;
    for(svci i = control_changes.begin(); i != control_changes.end(); ++i)
        {
        warning() << *i;
       }

but I guess I'm not so worried about constness there. I had to write
either "::iterator" or "::const_iterator", so I made the better choice.
But with Lavavej's proposal, I don't have to write any type or make any
choice. As he notes:

| Users with modifiable ranges who really want to view their elements as
| const can continue to say "for (const auto& elem : range)".

but I don't think I'd choose to do that here.

In this example:

    typedef std::map<std::string,std::string>::const_iterator smci;
    for(smci i = transfer_data_.begin(); i != transfer_data_.end(); ++i)
    ...
        transfer_data_       [name] = model_value;

I don't see a compelling reason for a const iterator, especially because
we modify the contents anyway despite the map's constness.

And here:

    typedef std::vector<wxWindow*>::const_iterator wvci;

what's the danger? I could still instantiate a new wxWindow*, so making
this iterator const would only prevent me from adding it to the list of
dialog controls accidentally. I don't need to be protected from that.

The other examples in that file all fit one of the patterns above.
This leads me to place a lower value on writing 'const' in a
for-range-declaration.

This is still different from Rule 4 because of the '&&', so I propose
a new...

Rule 5: Always write ranged-for loops as 'for(auto&& ...'.

...which is my new favorite. I'm supposing that 'auto&&' should
optimize at least as well as 'auto&', and probably better; and, AIUI,
it's a universal idiom that works well with template metaprogramming
too, and I'm not absolutely sure that we never need it even today.