Using references vs. pointers in C++ code

[Emilio Cobos Álvarez]

Hi dev-platform@,

So, yesterday was working on a bug (bug 1362991, if you're curious) when
I decided to do some spring cleanup and pass some non-optional argument
as a reference instead of as a pointer.

I got the cleanup patch rejected, because it went against the prevailing
style of the codebase, and I was told that moving to references required
a discussion in dev-platform, resulting in something more explicit in
the style guide.

So, I'll revert that patch for now, but I'm sending this mail to
kickstart that discussion :)

TL;DR: I think references are useful. We have encoded nullability of
arguments and return values implicitly or explicitly in different ways:
assertions inside of the function, naming conventions...

I think references help to encode that a bit more in the type system,
and help reasoning about the code without having to look at the
implementation of the function you're calling into, or without having to
rely on the callers to know that you expect a non-null argument.

Personally, I don't think that the fact that they're not used as much as
they could/should is a good argument to prevent their usage, but I don't
know what's the general opinion on that.

Thanks for reading.

-- Emilio

[Masayuki Nakano]

I like using reference to argument if it's non-nullable.
In Core::Editor module, such arguments are already replaced with
reference in a lot of places.

--
Masayuki Nakano <masa...@d-toybox.com>
Software Engineer, Mozilla

[Henri Sivonen]

On Tue, May 9, 2017 at 12:58 PM, Emilio Cobos Álvarez <emi...@crisal.io> wrote:
> I think references help to encode that a bit more in the type system,
> and help reasoning about the code without having to look at the
> implementation of the function you're calling into, or without having to
> rely on the callers to know that you expect a non-null argument.
>
> Personally, I don't think that the fact that they're not used as much as
> they could/should is a good argument to prevent their usage, but I don't
> know what's the general opinion on that.

The relevant bit of the Core Guidelines is
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-ptr-ref
and says:
"A pointer (T*) can be a nullptr and a reference (T&) cannot, there is
no valid "null reference". Sometimes having nullptr as an alternative
to indicated "no object" is useful, but if it is not, a reference is
notationally simpler and might yield better code."

As a result, I have an in-flight patch that takes T& instead of
NotNull<T*> where applicable, even though I do use NotNull<T*> to
annotate return values.

I agree that in principle it makes sense to use the type system
instead of relying on partial debug-build run-time measures to denote
non-null arguments when possible. That said, having to dereference a
smart pointer with prefix * in order to pass its referent to a method
that takes a T& argument feels a bit odd when one is logically
thinking of passing a pointer still, but then, again, &*foo seems like
common pattern on the Rust side of FFI to make a reference out of a
pointer and effectively asserting to the human reader that the pointer
is null.

--
Henri Sivonen
hsiv...@hsivonen.fi
https://hsivonen.fi/

[Mike Hommey]

Note that if you dereference a pointer to pass as a reference, all hell
breaks loose and your reference might just as well be null, but the
function taking the reference won't be protected against it because the
compiler will have assumed, when compiling it, that the reference can't
be null.

Mike

[Bobby Holley]

I think passing non-nullable things by reference is good, but I think we
should keep it consistent for a given type. So, for example, we shouldn't
have some callsites that take an nsPresContext* and others that take an
nsPresContext& (unless we have a rare case of a nullable presContext arg,
in which case |nsPresContext* aPresContextOrNull| would suffice).

Basically, I think consistency is important, but maintaining that
consistency per-type rather than system-wide seems like a reasonable
compromise. Curious as to what others think here.

bholley

On Tue, May 9, 2017 at 11:58 AM, Emilio Cobos Álvarez <emi...@crisal.io>
wrote:

> Hi dev-platform@,
>
> So, yesterday was working on a bug (bug 1362991, if you're curious) when
> I decided to do some spring cleanup and pass some non-optional argument
> as a reference instead of as a pointer.
>
> I got the cleanup patch rejected, because it went against the prevailing
> style of the codebase, and I was told that moving to references required
> a discussion in dev-platform, resulting in something more explicit in
> the style guide.
>
> So, I'll revert that patch for now, but I'm sending this mail to
> kickstart that discussion :)
>
> TL;DR: I think references are useful. We have encoded nullability of
> arguments and return values implicitly or explicitly in different ways:
> assertions inside of the function, naming conventions...
>

> I think references help to encode that a bit more in the type system,
> and help reasoning about the code without having to look at the
> implementation of the function you're calling into, or without having to
> rely on the callers to know that you expect a non-null argument.
>
> Personally, I don't think that the fact that they're not used as much as
> they could/should is a good argument to prevent their usage, but I don't
> know what's the general opinion on that.
>

> Thanks for reading.
>
> -- Emilio
>

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
>

[Eric Rescorla]

As Henri indicates, I think the use of references is consistent with
the style guide.

It's also worth noting that if you are using boxed pointers, then you
almost certainly want to use references to pass them around. I.e.,

foo(const RefPtr<T>& mPtr); // avoids useless ref count
foo(const UniquePtr<T>& mPtr); // avoids attempted (and failed)
ownership transfer.


-Ekr

P.S. The Google style guide prohibits non-const references. We follow this
in the parts of the code where we follow that style (e.g., the NSS
gtests).
https://google.github.io/styleguide/cppguide.html#Reference_Arguments


On Tue, May 9, 2017 at 3:31 AM, Henri Sivonen <hsiv...@hsivonen.fi> wrote:

> On Tue, May 9, 2017 at 12:58 PM, Emilio Cobos Álvarez <emi...@crisal.io>
> wrote:
> > I think references help to encode that a bit more in the type system,
> > and help reasoning about the code without having to look at the
> > implementation of the function you're calling into, or without having to
> > rely on the callers to know that you expect a non-null argument.
> >
> > Personally, I don't think that the fact that they're not used as much as
> > they could/should is a good argument to prevent their usage, but I don't
> > know what's the general opinion on that.
>

> The relevant bit of the Core Guidelines is
> https://github.com/isocpp/CppCoreGuidelines/blob/master/
> CppCoreGuidelines.md#Rf-ptr-ref
> and says:
> "A pointer (T*) can be a nullptr and a reference (T&) cannot, there is
> no valid "null reference". Sometimes having nullptr as an alternative
> to indicated "no object" is useful, but if it is not, a reference is
> notationally simpler and might yield better code."
>
> As a result, I have an in-flight patch that takes T& instead of
> NotNull<T*> where applicable, even though I do use NotNull<T*> to
> annotate return values.
>
> I agree that in principle it makes sense to use the type system
> instead of relying on partial debug-build run-time measures to denote
> non-null arguments when possible. That said, having to dereference a
> smart pointer with prefix * in order to pass its referent to a method
> that takes a T& argument feels a bit odd when one is logically
> thinking of passing a pointer still, but then, again, &*foo seems like
> common pattern on the Rust side of FFI to make a reference out of a
> pointer and effectively asserting to the human reader that the pointer
> is null.
>

> --
> Henri Sivonen
> hsiv...@hsivonen.fi
> https://hsivonen.fi/

[Nathan Froyd]

On Tue, May 9, 2017 at 5:58 AM, Emilio Cobos Álvarez <emi...@crisal.io> wrote:
> Personally, I don't think that the fact that they're not used as much as
> they could/should is a good argument to prevent their usage, but I don't
> know what's the general opinion on that.

The argument I have always heard, Gecko-wise and elsewhere [1], is to
prefer pointers for modification, because it's clearly signaled at the
callsite that something might be happening to the value. That would
rule out `T&`, but permit `const T&`.

-Nathan

[1] https://google.github.io/styleguide/cppguide.html#Reference_Arguments

[smaug]

This is what I'm a bit worried. We're moving some null checks from callee to caller, so need to
learn to be more careful with null checks on caller side.
But in general, using references sounds ok.


-Olli

[Boris Zbarsky]

On 5/9/17 9:03 AM, Bobby Holley wrote:
> I think passing non-nullable things by reference is good, but I think we
> should keep it consistent for a given type.

I should note that we already have this across all types to some extent:
WebIDL bindings pass non-nullable interface types as references, on
purpose, to make it clear that they can't be null (and conversely to
make it very clear which things _can_ be null and hence need null-checking).

In case it's not obvious, I support passing non-nullable things as
references. ;)

-Boris

[Boris Zbarsky]

On 5/9/17 9:17 AM, Nathan Froyd wrote:
> On Tue, May 9, 2017 at 5:58 AM, Emilio Cobos Álvarez <emi...@crisal.io> wrote:
>> Personally, I don't think that the fact that they're not used as much as
>> they could/should is a good argument to prevent their usage, but I don't
>> know what's the general opinion on that.
>
> The argument I have always heard, Gecko-wise and elsewhere [1], is to
> prefer pointers for modification

This is for primitive-typed out or inout params, right?

In other words, we should prefer "int*" to "int&" for places where we
expect the callee to modify the int, just like we should prefer
"MyClass**" to "MyClass*&". I guess the same for POD structs if we
expect people to be writing to them wholesale via assignment operators?
Not sure.

But for object-typed things like dom::Element or nsIFrame, it seems
better to me to pass references instead of pointers (i.e "Element&" vs
"Element*") for what are fundamentally in params, even though the callee
may call mutators on the passed-in object. That is, the internal state
of the element may change, but its identity will not.

-Boris

[smaug]

I could add still, that using references does add another thing to reviewers' checklist to ensure
that null pointer isn't ever dereferenced, and it also means that there will be more null checks, since
parameters need to be validated on callers' side, not in callee.

[Nathan Froyd]

On Tue, May 9, 2017 at 10:39 AM, Boris Zbarsky <bzba...@mit.edu> wrote:
> On 5/9/17 9:17 AM, Nathan Froyd wrote:
>> The argument I have always heard, Gecko-wise and elsewhere [1], is to
>> prefer pointers for modification
>
> This is for primitive-typed out or inout params, right?

I don't remember hearing any distinction one way or the other. I
don't think we have a rule written down for Gecko, but a lot of the
things we have historically dealt with are heap-allocated anyway or
have had to extensively deal with XPIDL, so passing around pointers
has seemed natural. But many recent things (e.g. WebIDL, some of the
editor refactorings, etc.) have started to prefer references even to
heap-allocated things, so we now have to think a little harder.

> In other words, we should prefer "int*" to "int&" for places where we expect
> the callee to modify the int, just like we should prefer "MyClass**" to
> "MyClass*&". I guess the same for POD structs if we expect people to be
> writing to them wholesale via assignment operators? Not sure.

I think a broader definition of "POD struct" would be required here:
RefPtr<T> and similar are technically not POD, but I *think* you'd
want to require RefPtr<T>* arguments when you expect the smart pointer
to be assigned into? Not sure.

> But for object-typed things like dom::Element or nsIFrame, it seems better
> to me to pass references instead of pointers (i.e "Element&" vs "Element*")
> for what are fundamentally in params, even though the callee may call
> mutators on the passed-in object. That is, the internal state of the
> element may change, but its identity will not.

I get this argument: you want the non-nullability of references, but
you don't want the verboseness (or whatever) of NonNull<T> or similar,
so you're using T& as a better T*. I think I would feel a little
better about this rule if we permitted it only for types that deleted
assignment operators. Not sure if that's really practical to enforce.

-Nathan

[Emilio Cobos Álvarez]

To be clear, I'm not suggesting to move functions that now do the
null-checking in the function itself to take references. I think
functions that take pointers and handle null are perfectly fine.

But doing functions that already don't null-check because they're
already checked at the callsite seem to me they could perfectly take a
reference.

Also, there's a case for the opposite I think, where taking references
make reviewing easier.

To give one example, I just grepped a bit and found
EffectCompositor::GetAnimationElementAndPseudoForFrame[1] (there are a
lot of other functions like that, like all the IsFoo functions in the
frame constructor, etc).

In any case: That function dereferences the aFrame parameter without
null-checking, not even asserting.

If someone is, hypothetically, adding a caller to that function, and
calls it with a frame pointer that may be null, I think that function
taking a reference would, in any case, make the job of the reviewer
_easier_: It makes the explicit dereference at the callsite, so the
reviewer can evaluate whether that dereference is correct.

Otherwise, the reviewer would need to check the implementation of
GetAnimationElementAndPseudoForFrame in order to realise that it doesn't
handle null.

It's kind of a silly example (because that function itself doesn't make
a lot of sense if you don't give it a frame), but I can find some more
realistic examples of the same situation if you want to.

-- Emilio

[1]: http://searchfox.org/mozilla-central/source/dom/animation/EffectCompositor.cpp#704

[Boris Zbarsky]

On 5/9/17 11:41 AM, Nathan Froyd wrote:
> I think I would feel a little
> better about this rule if we permitted it only for types that deleted
> assignment operators. Not sure if that's really practical to enforce.

Hmm. I wonder what happens right now if you try to invoke
nsINode::operator=....

But yes, having such a restriction would make sense to me if we can do it.

-Boris

[L. David Baron]

On Tuesday 2017-05-09 11:58 +0200, Emilio Cobos Álvarez wrote:
> I think references help to encode that a bit more in the type system,
> and help reasoning about the code without having to look at the
> implementation of the function you're calling into, or without having to
> rely on the callers to know that you expect a non-null argument.
>
> Personally, I don't think that the fact that they're not used as much as
> they could/should is a good argument to prevent their usage, but I don't
> know what's the general opinion on that.

I have two general concerns about references:

(1) When reading the calling code that calls a function taking a
non-const reference, it's not obvious that the function might
mutate the value, particularly in code that uses references
rarely, or for types that are rarely passed as references.

(2) Code that mixes functions that take references and functions
that take pointers, for the same time, is a mess, because you
constantly have to worry about whether to write * or &. Thus,
in Gecko, we generally use pointers or references per-type.
XPCOM interfaces are passed by pointer. String classes are
passed by reference. etc.

I think per-type conventions for using references versus pointers
helps with both (1) and (2), and I consider it part of Gecko style
even if it's not well-documented.

I'm also not sure how much references help with enforcing
non-null-ness. People will just write a "*" whether or not their
existing code guarantees a non-null pointer; at least that was my
experience in the early days of Gecko. Perhaps we have better code
review now... but I'm somewhat skeptical of that given that I think
we've been relying on tests more and review less?

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla https://www.mozilla.org/ 𝄂
Before I built a wall I'd ask to know
What I was walling in or walling out,
And to whom I was like to give offense.
- Robert Frost, Mending Wall (1914)

[Michael Layzell]

It wouldn't be too difficult to have a static analysis which complains if a
RefCounted object (An object with AddRef and Release methods) is assigned
into at all, with the assumption that that will do The Wrong Thing™ 100% of
the time.

This of course would only be caught on infrastructure because most people
don't run the SA locally (we have a bug to make this easier which Andi is
working on), but that might be OK as none of this code would get into the
tree.


>
>
> -Boris

[Karl Tomlinson]

Nathan Froyd writes:
> I think a broader definition of "POD struct" would be required here:
> RefPtr<T> and similar are technically not POD, but I *think* you'd
> want to require RefPtr<T>* arguments when you expect the smart pointer
> to be assigned into? Not sure.

Yes, please, for similar reasons as for the primitive types.

>On Tue, May 9, 2017 at 10:39 AM, Boris Zbarsky <bzba...@mit.edu> wrote:
>> But for object-typed things like dom::Element or nsIFrame, it seems better
>> to me to pass references instead of pointers (i.e "Element&" vs "Element*")
>> for what are fundamentally in params, even though the callee may call
>> mutators on the passed-in object. That is, the internal state of the
>> element may change, but its identity will not.

Nathan Froyd writes:
> I get this argument: you want the non-nullability of references, but
> you don't want the verboseness (or whatever) of NonNull<T> or similar,
> so you're using T& as a better T*. I think I would feel a little
> better about this rule if we permitted it only for types that deleted
> assignment operators. Not sure if that's really practical to enforce.

With convention to use references to objects for methods that
change the state of the object, the advantages are

R1. Nullability is indicated differently from NotNull<T>.

R2. Parameters which may change the identity of an object can be
distinguished at the call site from those that may change only
state.

With convention to use pointers to objects for methods that change
the state of the object, the advantages are

P1. Parameters which change the state of an object can be
distinguished at the call site from those that don't.

P2. Consistency with primitive types.

My experience is that for understanding while reading code, it is
more helpful to distinguish where state may be modified than to
distinguish what may be null. One particular case is where a
method transfers state from one object to another. At the call
site it is useful to distinguish source from destination.

For object types, change in identity is rare (non-existent for
ref-counted objects) and so there is rarely a need to distinguish
this from state change. Typically identity changes only when the
pointer changes.

Doesn't NotNull<T> provide that pointers can have the same
advantages as references re visibility of nullability where
desired, so we can have the best of both?

Or is NotNull really too awkward IRL?

[Emilio Cobos Álvarez]

On Tue, May 09, 2017 at 02:11:41PM -0700, L. David Baron wrote:
> On Tuesday 2017-05-09 11:58 +0200, Emilio Cobos Álvarez wrote:
> > I think references help to encode that a bit more in the type system,
> > and help reasoning about the code without having to look at the
> > implementation of the function you're calling into, or without having to
> > rely on the callers to know that you expect a non-null argument.
> >
> > Personally, I don't think that the fact that they're not used as much as
> > they could/should is a good argument to prevent their usage, but I don't
> > know what's the general opinion on that.
>
> I have two general concerns about references:
>
> (1) When reading the calling code that calls a function taking a
> non-const reference, it's not obvious that the function might
> mutate the value, particularly in code that uses references
> rarely, or for types that are rarely passed as references.

Isn't it similarly non-obvious for code that takes non-const pointers
when the object isn't in the stack?

For example, if I have `void foo(Element*)`, which calls
`void bar(Element*)`, and both assume a non-null argument, I don't see
how potential mutation is more obvious there than `void foo(Element&)`
calling `void bar(Element&)`.

(Other people are concerned about non-const references used for
outparams. I think I'll be fine with the rule Nathan proposed in the
thread as a rule of thumb).

> (2) Code that mixes functions that take references and functions
> that take pointers, for the same time, is a mess, because you
> constantly have to worry about whether to write * or &.

I agree with this, though I don't think writing & or * is a big deal in
most cases.

> Thus, in Gecko, we generally use pointers or references per-type.
> XPCOM interfaces are passed by pointer. String classes are
> passed by reference. etc.
>
> I think per-type conventions for using references versus pointers
> helps with both (1) and (2), and I consider it part of Gecko style
> even if it's not well-documented.

The issue I have with per-type conventions is that it doesn't scale very
well, specially when you're writing new code (and more specially if
they're not documented in the style guide).

What should the convention be for `ServoStyleSet` (which is what
triggered this thread)? Who decides that, and based on which arguments?

> I'm also not sure how much references help with enforcing
> non-null-ness. People will just write a "*" whether or not their
> existing code guarantees a non-null pointer; at least that was my
> experience in the early days of Gecko. Perhaps we have better code
> review now... but I'm somewhat skeptical of that given that I think
> we've been relying on tests more and review less?

I'm not talking about enforcing non-null-ness, because as you've pointed
out, references don't necessarily do that.

I think the main benefit is at the time of both reading and reasoning
about the code. Using references allows you to quickly figure out what
kind of thing a function expects, handles, or returns, without having to
look at the function itself and potentially all the callers.

-- Emilio

[L. David Baron]

On Wednesday 2017-05-10 02:43 +0200, Emilio Cobos Álvarez wrote:
> The issue I have with per-type conventions is that it doesn't scale very
> well, specially when you're writing new code (and more specially if
> they're not documented in the style guide).
>
> What should the convention be for `ServoStyleSet` (which is what
> triggered this thread)? Who decides that, and based on which arguments?

In this particular case, I think it's easy. ServoStyleSet is the
Stylo version of a class (nsStyleSet) that uses pointer conventions
because it used to implement XPCOM interfaces. We shouldn't treat
the two style set classes differently. So it should be passed with
pointers, not references.

[Nicholas Nethercote]

On Wed, May 10, 2017 at 10:27 AM, Karl Tomlinson <moz...@karlt.net> wrote:

>
> Or is NotNull really too awkward IRL?
>

I wrote NotNull.h, and I've used it in various places. I'm ambivalent about
it. It does make things clear, but it also is a bit annoying to use. The
code tends to end up with WrapNotNull() and get() calls littered
throughout. It also doesn't currently work with UniquePtr.

The two big comments in mfbt/NotNull.h have some discussion about use cases
for references and pointers and NotNull that is relevant to this thread:
https://dxr.mozilla.org/mozilla-central/rev/b21b974d60d3075ae24f6fb1bae75d0f122f28fc/mfbt/NotNull.h#10-99

I also fiddled around with a MaybeNull type, to explicitly mark pointers
that are nullable, but it was more complex and less obviously useful than
NotNull, so I gave up on it.

Nick

[Emilio Cobos Álvarez]

On Tue, May 09, 2017 at 06:56:03PM -0700, L. David Baron wrote:
> On Wednesday 2017-05-10 02:43 +0200, Emilio Cobos Álvarez wrote:
> > The issue I have with per-type conventions is that it doesn't scale very
> > well, specially when you're writing new code (and more specially if
> > they're not documented in the style guide).
> >
> > What should the convention be for `ServoStyleSet` (which is what
> > triggered this thread)? Who decides that, and based on which arguments?
>
> In this particular case, I think it's easy. ServoStyleSet is the
> Stylo version of a class (nsStyleSet) that uses pointer conventions
> because it used to implement XPCOM interfaces. We shouldn't treat
> the two style set classes differently. So it should be passed with
> pointers, not references.

I didn't know that nsStyleSet was previously an XPCOM interface. Does
that mean that stuff like nsStyleContext, etc shouldn't be
passed/returned by reference either in your opinion? (just realised it
was also an XPCOM interface at the time).

I think we shouldn't need to do code archeology in order to find out if
we can use references or not in the code.

-- Emilio

> -David
>
> --
> 𝄞 L. David Baron http://dbaron.org/ 𝄂
> 𝄢 Mozilla https://www.mozilla.org/ 𝄂
> Before I built a wall I'd ask to know
> What I was walling in or walling out,
> And to whom I was like to give offense.
> - Robert Frost, Mending Wall (1914)

[L. David Baron]

On Wednesday 2017-05-10 13:10 +0200, Emilio Cobos Álvarez wrote:
> On Tue, May 09, 2017 at 06:56:03PM -0700, L. David Baron wrote:
> > On Wednesday 2017-05-10 02:43 +0200, Emilio Cobos Álvarez wrote:
> > > The issue I have with per-type conventions is that it doesn't scale very
> > > well, specially when you're writing new code (and more specially if
> > > they're not documented in the style guide).
> > >
> > > What should the convention be for `ServoStyleSet` (which is what
> > > triggered this thread)? Who decides that, and based on which arguments?
> >
> > In this particular case, I think it's easy. ServoStyleSet is the
> > Stylo version of a class (nsStyleSet) that uses pointer conventions
> > because it used to implement XPCOM interfaces. We shouldn't treat
> > the two style set classes differently. So it should be passed with
> > pointers, not references.
>
> I didn't know that nsStyleSet was previously an XPCOM interface. Does
> that mean that stuff like nsStyleContext, etc shouldn't be
> passed/returned by reference either in your opinion? (just realised it
> was also an XPCOM interface at the time).
>
> I think we shouldn't need to do code archeology in order to find out if
> we can use references or not in the code.

It doesn't require code archaeology; it just requires observing that
all the existing code uses pointers rather than references, and
therefore that you should be consistent with that.