Sorry for the empty subject line earlier...
Jason Merrill <ja...@redhat.com> writes:
> On Fri, May 15, 2020 at 9:47 PM Martin Sebor <mse...@gmail.com> wrote:
>
>> On 5/15/20 8:08 AM, Richard Sandiford wrote:
>> >> Those are all good examples. Mind putting that into a patch
>> >> for the coding conventions?
>> > How's this? I added "new" expressions as another example of the
>> > first category.
>> >
>> > I'm sure I've missed other good uses, but we can always add to the
>> > list later if necessary.
>> >
>> > Thanks,
>> > Richard
>> >
>> >
>> > 0001-Describe-coding-conventions-surrounding-auto.patch
>> >
>> > From 10b27e367de0fa9d5bf91544385401cdcbdb8c00 Mon Sep 17 00:00:00 2001
>> > From: Richard Sandiford<richard.sandif...@arm.com>
>> > Date: Fri, 15 May 2020 14:58:46 +0100
>> > Subject: [PATCH] Describe coding conventions surrounding "auto"
>> >
>> > ---
>> > htdocs/codingconventions.html | 53 +++++++++++++++++++++++++++++++++++
>> > htdocs/codingrationale.html | 17 +++++++++++
>> > 2 files changed, 70 insertions(+)
>> >
>> > diff --git a/htdocs/codingconventions.html
>> b/htdocs/codingconventions.html
>> > index f4732ef6..ae49fb91 100644
>> > --- a/htdocs/codingconventions.html
>> > +++ b/htdocs/codingconventions.html
>> > @@ -51,6 +51,7 @@ the conventions separately from any other changes to
>> the code.</p>
>> > <li><a href="#Cxx_Language">Language Use</a>
>> > <ul>
>> > <li><a href="#Variable">Variable Definitions</a></li>
>> > + <li><a href="#Auto">Use of <code>auto</code></a></li>
>> > <li><a href="#Struct_Use">Struct Definitions</a></li>
>> > <li><a href="#Class_Use">Class Definitions</a></li>
>> > <li><a href="#Constructors">Constructors and
>> Destructors</a></li>
>> > @@ -884,6 +885,58 @@ Variables may be simultaneously defined and tested
>> in control expressions.
>> > <a href="codingrationale.html#variables">Rationale and Discussion</a>
>> > </p>
>> >
>> > +<h4 id="Auto">Use of <code>auto</code></h4>
>> > +
>> > +<p><code>auto</code> should be used in the following circumstances:
>> > +<ul>
>> > + <li><p>when the expression gives the C++ type explicitly. For
>> example</p>
>> > +
>> > + <blockquote>
>> > +<pre>if (<b>auto *</b>table = dyn_cast <<b>rtx_jump_table_data
>> *</b>> (insn)) // OK
>> > + ...
>> > +if (rtx_jump_table_data *table = dyn_cast <rtx_jump_table_data *>
>> (insn)) // Avoid
>> > + ...
>> > +<b>auto *</b>map = new <b>hash_map <tree, size_t></b>;
>> // OK
>> > +hash_map <tree, size_t> *map = new hash_map <tree, size_t>;
>> // Avoid</pre></blockquote>
>> > +
>> > + <p>This rule does not apply to abbreviated type names embedded in
>> > + an identifier, such as the result of <code>tree_to_shwi</code>.</p>
>> > + </li>
>> > + <li>
>> > + <p>when the expression simply provides an alternative view of an
>> object
>> > + and is bound to a read-only temporary. For example:</p>
>> > +
>> > + <blockquote>
>> > +<pre><b>auto</b> wioff = <b>wi::to_wide (off);</b> // OK
>> > +wide_int wioff = wi::to_wide (off); // Avoid if wioff is read-only
>> > +<b>auto</b> minus1 = <b>std::shwi (-1, prec);</b> // OK
>> > +wide_int minus1 = std::shwi (-1, prec); // Avoid if minus1 is
>> read-only</pre></blockquote>
>> > +
>> > + <p>In principle this rule applies to other views of an object too,
>> > + such as a reversed view of a list, or a sequential view of a
>> > + <code>hash_set</code>. It does not apply to general
>> temporaries.</p>
>> > + </li>
>> > + <li>
>> > + <p>the type of an iterator. For example:</p>
>> > +
>> > + <blockquote>
>> > +<pre><b>auto</b> it = <b>std::find (names.begin (), names.end (),
>> needle)</b>; // OK
>> > +vector <name_map>::iterator it = std::find (names.begin (),
>> > + names.end (), needle); //
>> Avoid</pre></blockquote>
>> > + </li>
>> > + <li>
>> > + <p>the type of a lambda expression. For example:</p>
>> > +
>> > + <blockquote>
>> > +<pre><b>auto</b> f = <b>[] (int x) { return x + 1; }</b>; //
>> OK</pre></blockquote>
>> > + </li>
>> > +</ul></p>
>> > +
>> > +<p><code>auto</code> should <b>not</b> be used in other contexts.</p>
>>
>> This seems like a severe (and unnecessary) restriction...
>>
>> > +
>> > +<p>
>> > +<a href="codingrationale.html#auto">Rationale and Discussion</a>
>> > +</p>
>> >
>> > <h4 id="Struct_Use">Struct Definitions</h4>
>> >
>> > diff --git a/htdocs/codingrationale.html b/htdocs/codingrationale.html
>> > index 0b44f1da..a919023c 100644
>> > --- a/htdocs/codingrationale.html
>> > +++ b/htdocs/codingrationale.html
>> > @@ -50,6 +50,23 @@ if (info *q = get_any_available_info ()) {
>> > }
>> > </code></pre></blockquote>
>> >
>> > +<h4 id="auto">Use of <code>auto</code></h4>
>> > +
>> > +<p>The reason for preferring <code>auto</code> in expressions like:
>> > +<blockquote><pre>auto wioff = wi::to_wide (off);</pre></blockquote>
>> > +is that using the natural type of the expression is more efficient than
>> > +converting it to types like <code>wide_int</code>.</p>
>> > +
>> > +<p>The reason for excluding other uses of <code>auto</code> is that
>> > +in most other cases the type carries useful information. For example:
>> > +<blockquote><pre>for (const std::pair <const char *, tree>
>> &elt : indirect_pool)
>> > + ...</pre></blockquote>
>> > +makes it obvious that <code>elt</code> is a pair and gives the types of
>> > +<code>elt.first</code> and <code>elt.second</code>. In contrast:
>> > +<blockquote><pre>for (const auto &elt : indirect_pool)
>> > + ...</pre></blockquote>
>> > +gives no immediate indication what <code>elt</code> is or what can
>> > +be done with it.</p>
>>
>> ...there are countless constructs in C++ 98 as well in C where there
>> is no such indication yet we don't (and very well can't) try to avoid
>> using them. Examples include macros, members of structures defined
>> far away from the point of their use, results of ordinary function
>> calls, results of overloaded functions or templates, default function
>> arguments, default template parameters, etc.
>>
>> By way of a random example from genrecog.c:
>>
>> int_set::iterator end
>> = std::set_union (trans1->labels.begin (), trans1->labels.end (),
>> combined->begin (), combined->end (),
>> next->begin ());
>>
>> There is no immediate indication precisely what type int_set::iterator
>> is. All we can tell is that that it's some sort of an iterator, and
>> that should be good enough. It lets us (even forces us to) write code
>> that satisfies the requirements of the abstraction (whatever it happens
>> to be), and avoid tying it closely to the implementation. That's
>> a good thing.
Do you mean that this example should or shouldn't use "auto"?
Iterators are included in the list above, so the idea was that using
"auto" would be the recommended style here.
>> Unless there is a sound technical reason for avoiding it (e.g.,
>> unacceptable inefficiency or known safety problems) I'd say leave
>> it to everyone's judgment what convenience features to use. If
>> something turns out to be a problem we'll deal with it then.
But using "auto" is never going to be an efficiency concern,
and probably not a safety concern. So in the case of "auto",
using that principle would basically come down to "when to use
auto is purely a judgement call".
I don't see how we can get consistency with that kind of approach.
Or is the argument that we're (or I'm :-)) worrying too much about
consistency and we should just go with the flow?
If we do treat it as a pure judgement call, the problem then is:
who's judgement matters most here? The author's or the reviewer's?
Should the reviewer respect the choice of the author even if they
don't personally agree with it, given that there are no technical
issues at stake?
> I agree with this. If using 'auto' makes the code harder to read, that's a
> good comment for code review, but it's hard to write a general rule for
> this sort of thing.
I think the downsides of a pure "it's up to the reviewer" approach are:
- Every contributor and reviewer has their own opinions. The point of
coding standards is to aim for consistency in a large project despite
these various and often conflicting opinions.
- If it is (or gives the impression of being) purely down to the
individual opinions of the reviewer, things can get awkward.
E.g. -- and maybe I shouldn't use a personal example like this --
I remember Martin S has complained a few times when, in code review,
Jeff has asked for changes to match existing GCC style (e.g. to use
upper case instead of CamelCase for enum values). In all the examples
I saw, Jeff was asking for what I understood the GCC style to be.
But Martin gave the impression of being quite annoyed at having to
make these changes, talking in the "enum case" about having to learn
individual reviewers' preferences.
I think that kind of reaction is inevitable if we don't try to agree
common standards. If things aren't written down, requests from reviewers
not to use a feature can seem arbitrary or whimsical even when they're not.
The same goes for requests to follow conventions that have built up
over time without being written down. The temptation can be to push
back against code review comments that aren't purely technical in nature.
If we take this approach, then over time (because of the different
opinions of different contributors) there is likely to be code in the
codebase that uses "auto" in the way that any given person wants to
use it. It will seem even more arbitrary to reject the use of "auto"
in one context if it has already been used elsewhere in another
similar context. So over time, we'll either get more review friction
or come to an understanding that the use of "auto" isn't a suitable
comment for code review after all.
I think the latter is more likely. It can even be framed positively:
reviewers should respect the choice of author to use "auto" whenever
they like, and not impose their own opinions.
But while individual patches are one source of hard-to-read code,
another is when different styles build up over time. And that's what
I'd really like to avoid.
Having a consistent style in itself makes code easier to read.
It also makes it easier to modify, because there's less guesswork
involved.
Like I say, this list isn't supposed to be fixed for all time.
We can add to it whenever a good missing case crops up. And the
template one that you (Jason) mentioned in the earlier message is
definitely a good one :-)
And to be clear, this is in no way an "anti-auto" drive. It's purely
about taking one feature that is (rightly) going to be used often
and trying to add some consistency and predictability around it.
Thanks,
Richard
