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

[Martin Sebor <msebor at gmail dot com>]

On 07/16/2018 02:19 AM, Richard Sandiford wrote:
> Aldy Hernandez <aldyh@redhat.com> writes:
> > On Thu, Jul 12, 2018 at 1:41 PM Jonathan Wakely <jwakely.gcc@gmail.com> 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 (an
> > > alternative to pointers is to return a struct with the wide int result
> > > and the overflow flag), but ...
> >
> > Putting everything in a new struct is just going through more hoops to
> > avoid a common language idiom that everyone in C++ is used to.
> >
> > > > +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.
> >
> > Agreed.
> >
> > Richard (Sandiford), this really looks like going out of your way to
> > enforce a personal style issue across the entire code base.  It's sad
> > that we try to come up with new ways to make it even harder for new
> > developers to contribute to GCC.
>
> It's not enforcing a personal style issue so much as trying to stop
> the source base from becoming even more inconsistent, admittedly in one
> particular area only.
>
> Before the switch to C++, GCC was one of the most consistent source
> bases I'd seen (more so even than LLVM IMO, having worked on both).
> It's clearly less so now.  The problem isn't just legacy C-era code vs.
> new C++ code, but different styles being used for the C++ code.
>
> (It was interesting that Martin complained earlier in the thread about
> how inconsistent GCC was.  I imagine that's the impression that new
> developers would have now, rather than being glad that they can use
> references to return things.)

Just to be clear: mine wasn't meant to be a complaint so much
as an observation.  With a code base as large as GCC and with
so many people contributing to it, I wouldn't expect too much
more consistency unless it was enforced by tools.

I don't have a problem with adopting a new convention per se.
What I find a poor use of time and effort is relying on code
review as the only enforcement of a consistent coding style.
It gets especially frustrating when the same piece of code
that is acceptable to one reviewer is not good enough for
another only because it doesn't follow some convention that
GCC isn't consistent with to begin with (which to me seems
like it's more than those GCC is consistent with).

Martin

PS Here are some examples of documented GCC coding conventions
that I have noticed are either very inconsistent or virtually
ignored ignored (this isn't a survey):

  *  logical not    !x        vs  ! x      thousands of violations
  *  cast           (foo) x   vs  (foo)x   ditto
  *  C++-style casts vs C-style            mostly ignored
  *  explicit ctors                        ignored
  *  in class member definitions