Re: [RFC] Update coding conventions to restrict use of non-const references

[Richard Sandiford <richard dot sandiford at arm dot com>]

Pedro Alves <palves@redhat.com> writes:
> On 07/12/2018 12:40 PM, Jonathan Wakely wrote:
>> On Thu, 12 Jul 2018 at 11:41, Richard Sandiford wrote:
>>> +Only use non-constant references in the following situations:
>>> +</p>
>>> +
>>> +<ul>
>>> +<li>when they are necessary to conform to a standard interface, such as
>>> +the first argument to a non-member <code>operator+=</code></li>
>> 
>> And the return value of such operators (which also applies to member
>> operators, which is the more conventional way to write compound
>> assignment operators).
>> 
>>> +<li>in a return value, when providing access to something that is known
>>> +to be nonnull</li>
>>> +</ul>
>>> +
>>> +<p>
>>> +In other situations the convention is to use pointers instead.
>>> +</p>
>>> +
>>> +<blockquote>
>>> +<pre><code>HOST_WIDE_INT do_arith (..., bool *overflow);   // OK
>>> +HOST_WIDE_INT do_arith (..., bool &amp;overflow);   // Please avoid
>> 
>> I understand the objection to using references for out parameters 
>
> FWIW, GDB's conventions (which import GCC's coding conventions) already
> suggest avoiding non-const reference output parameters, so this won't
> affect us.
>
> But, FWIW, personally, while I used to be all for avoiding non-const
> reference output parameters, I no longer am, at least so strongly,
> nowadays.
>
> The reason being that the visual distinction can be easily lost with
> pointers too anyway:
>
>  // the usual argument is that using pointers for output parameters shows
>  // clearly that bar is going to be modified:
>  function (foo, &bar);
>
>  // but well, we often works with pointers, and if "bar" is a pointer,
>  // we don't get any visual clue anyway either:
>  function (foo, bar);

True, and I'm not claiming that code using pointers always makes
this obvious.  But like you say, both approaches lack a visual clue
in this case.  The pointer approach does at least mean we get an
error if "bar" isn't a pointer, whereas with references it would
compile if "bar" is a local object or a reference.

So with the pointer approach, there is at least an explicit statement of
intent somewhere in the calling code, even if it isn't at the call site
itself.

>  // which suggests that what really helps is seeing the output
>  // variable nearby, suggesting to define it right before the
>  // function call that fills it in, and I would go as far
>  // as saying that clearer symbol names help even more.  For e.g.:
>  B bar_out;
>  fill_bar (foo, bar_out);
>
> (an
>> alternative to pointers is to return a struct with the wide int result
>> and the overflow flag),
>
> +1.  I've been pushing GDB in that direction whenever possible.

I agree that can sometimes be better.  I guess it depends on the
context.  If a function returns a bool and some other data that has no
obvious "failure" value, it can be easier to write chained conditions if
the function returns the bool and provides the other data via an output
parameter.

>> but ...
>> 
>>> +int *elt = &amp;array[i];  // OK
>>> +int &amp;elt = array[i];   // Please avoid
>> 
>> ... this seems unnecessary. If the function is so long that the fact
>> elt is a reference can easily get lost, the problem is the length of
>> the function, not the use of a reference.
>> 
>
> +1000.  This seems really unnecessary.  References have the advantage
> of implicit never-null semantics, for instance.

The nonnullness is there either way in the above example though.

I don't feel as strongly about the non-const-reference variables, but for:

     int &elt = array[i];
     ...
     elt = 1;

it can be easy to misread that "elt = 1" is changing more than just
a local variable.

I take Jonathan's point that it shouldn't be much of a problem if
functions are a reasonable size, but we've not tended to be very
good at keeping function sizes down.  I guess part of the problem
is that even functions that start off small tend to grow over time.

We have been better at enforcing more specific rules like the ones
in the patch.  And that's easier to do because a patch either adds
references or it doesn't.  It's harder to force (or remember to force)
someone to split up a function if they're adding 5 lines to one that is
already 20 lines long.  Then for the next person it's 25 lines long, etc.

Also, the "int *elt" approach copes with cases in which "elt" might
have to be redirected later, so we're going to need to use it in some
cases.  It's then a question of whether "int &elt" is useful enough that
it's worth accepting it too, and having a mixture of styles in the codebase.

Thanks,
Richard