Re: [lmi] Request for review

[Greg Chicares]

On 2017-01-12 22:50, Vadim Zeitlin wrote:
> On Thu, 12 Jan 2017 18:36:01 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> Let me ask one last question before I start working on this patch. STL's
> GC> proposal above has two distinct components:
> GC> 
> GC>  - always use the same for-range-declaration (regardless of constness);
> GC> 
> GC>  - always write that One True for-range declaration with two ampersands.
> GC> 
> GC> Are you saying "one ampersand good, two ampersands bad" for our general
> GC> rule--that it's the '&&' specifically that makes it harder to resolve?
> 
>  My main problem with "auto&&" is that it loses "const" and so doesn't
> indicate the intention of the code to not modify *or* modify the container.
> Both are important and without one we can't have the other.

Then we can't write anything like 'auto const&&' or 'auto &const&'?

But don't answer that; I wouldn't understand the answer. (It reminds me
of the time a colleague was entering a command on an IBM 5150 and I,
my experience being limited to the 5110, asked him what the backslash
character meant. I'm sure he explained that it referred to a directory,
but without the "directory" concept I couldn't understand his answer.)

Besides, if we ever want to change this again, to 'auto ~:' in C++29,
e.g., these loops will have been written so uniformly that they'll be
easier to change at that time.

>  Additionally, seeing "auto&&" in non-template code makes me stop and
> wonder: why is it used here? As the only case in which it needs to be used
> is when the iterator dereferences to a proxy object instead of directly to
> the object itself (the representative example being iterating over the
> notorious std::vector<bool>), I think it will be puzzling to see it used
> when iterating over std::vector<double> or std::string and even more so
> when iterating over fs::directory_iterator where things are less obvious
> (but "auto&&" is still not needed).

/opt/lmi/free/src/lmi[0]$grep 'vector *< *bool' *.?pp       
account_value.hpp:    // Prefer int here because vector<bool> is not a 
container.

Whew. Only a comment and nothing more.

Although it shouldn't have been associated with std::vector<bool>, a bitset
with one bit of storage per bit of boolean data is a great idea. We had it on
the IBM 5110 (I'm just sayin').

> GC> (I can't claim to understand it well yet, but I had the impression that it
> GC> would give us more efficient "move" semantics, at least in some cases;
> 
>  No, not at all, there is no moving involved here anyhow (nor copying). In
> the case of "normal" iterators both "auto&" and "auto&&" are equally
> efficient and both are just references (so no copying/moving) and in the
> case of iterators returning proxies (std::vector<bool> case), "auto&"
> simply won't compile.

Even better, then: if we ever needed a double ampersand, the compiler would
insist.

> GC> If so, then it's agreed: we'll simply use Rule 2:
> 
>  I'm not sure to follow the exact logic leading to it, but I definitely
> agree with the conclusion.

Citing its original statement again:

- for(std::vector<T>::iterator ...
+ for(auto& ...
- for(std::vector<T>::const_iterator ...
+ for(auto const& ...

IOW, when translating a non-ranged 'for' to a ranged 'for':
  for ( for-range-declaration : expression ) statement
where
  for-range-declaration: attribute-specifier-seq[opt] decl-specifier-seq 
declarator
our rule requires:
 (1) no attribute-specifier-seq shall be used;
 (2) the decl-specifier-seq shall be either 'auto' or 'auto const',
     whichever is more appropriate in context;
 (3) the declarator shall include the '&' ref-qualifier.

Rationale:

(1) We don't care about the alignment (e.g.) of iterators.

(2) 'auto' is preferred to the actual type for uniformity (all ranged-for
constructs are written the same way) and robustness (if the container's
datatype changes, an explicitly-specified actual type would need to be
edited; but the deduced type adapts automatically, and the compiler
guarantees that it is correct). 'const' is to be written wherever
appropriate because it expresses the intent clearly, and allows us to
write a for-statement that cannot alter its container even if the
container itself is not physically const.

(3) Const references cost nothing (according to your tests) if the deduced
type is primitive; otherwise, they save the overhead of copying. Non-const
references, where written through, are generally required for correctness
(and, if not written through, they should be const anyway).

>  Please let me know if I'm missing something here (I have the impression I
> do, but really no idea what...),

I hope that the rationale shows that you're missing nothing.

Let me present the rule in terms of its effects: it requires modifications
in the following changes in each patch (sorted, so we can see that lines
containing 'auto' lack '&' (violating (3) above), and the other lines
lack 'auto' (violating (2) above).

# automated clang-tidy patch

$grep '+.*for(' a00/patches/52a.patch | sed -e'/for(auto&/d' -e'/for(auto 
const&/d' -e's/^+ *for/for/' |sort     
for(auto c: s)
for(auto c: w->GetChildren())
for(auto c: weird_report_columns)
for(auto c: weird_report_columns)
for(auto i = fields.begin(); i != fields.end();)
for(auto i: selection)
for(auto m: moduli)
for(auto p: document.GetViews())
for(auto pw: Lineage(&CurrentPage()))
for(auto pw: lineage_)
for(auto pw: lineage_)
for(auto pw: lineage_)
for(auto run_base: ledger_->GetRunBases())
for(auto run_basis: RunBases)
for(auto uc: u)
for(auto w: Lineage(&CurrentPage()))
for(auto w: frame_->GetChildren())
for(auto w: frame_->GetChildren())
for(auto w: parent->GetChildren())
for(char c: original_text)
for(char c: raw_text)
for(char c: z)
for(double d: dv)
for(double vi : v)
for(int row: summary_rows)

$grep '+.*for(' a00/patches/52b.patch | sed -e'/for(auto&/d' -e'/for(auto 
const&/d' -e's/^+ *for/for/' |sort
for(auto field: fields)
for(auto rm: to_remove)
for(auto uc: vuc)
for(char c: s)
for(char& c: t)
for(double vi: v)
for(fs::path const& p: get_test_files_path())
for(fs::path const& p: path)
for(fs::path const& p: path)
for(fs::path const& p: path)
for(int pc: p)