Using references for known non-null pointers

[Adam Klein]

I've seen a lot of changes going by over the last several weeks
changing, e.g., Node::document() now returns a Document& instead of
Document*. My understanding was that these are being ported over from
similar work in WebKit, but I was surprised that there wasn't a
discussion about this change on blink-dev, especially given that I've
seen comments on code reviews from some Blink OWNERS (at least esprehn
and eseidel, CCed) that they're not sure the current approach is
ideal.

So, consider this a place to have that discussion. I'd hope that
besides broadening knowledge and consensus about the right way of
making this change in the codebase, this thread would also end up
giving us a few sentences to put in the style guide governing when to
use pointers vs references.

- Adam

[Eric Seidel]

My concerns may be out of ignorance. One concern I had was that Ref
(the RefPtr equivalent for references) seemed rather ugly. Ref == Ref
doesn't work, so you have to do ref.get() == ref.get().

It also feels a bit awkward to me to have a reference to a RefCounted
object. It's never safe to have a RefCounted object on the stack
itself. When I read a something passed by reference, I assume it's
stack allocated and that I can't hold onto it. That's obviously not
the case for Document&.

[TAMURA, Kent]

I'm sorry that I started some work without any notification.

I converted some arguments and return values in Page and the editing area. I found no downsides in these areas because

 - Ownership is very clear.  e.g. Page owns a FocusController.

 - No need to compare objects.

I removed a lot of unnecessary NULL checks, and it must have reduced binary size and source code size, and we don't need to worry about NULL deferences of these objects any longer.

I think it's worth to proceed the reference-conversion for objects handled by OwnPtr or similar relationship.

The difference between the above case and DOM nodes:

 - DOM node values are never null in some cases.  e.g. Node::document() and Document argument of Node factories

 - DOM node values can be null in some cases.  e.g. Document::focusedElement() can be null.

 - We need to compare DOM nodes frequently.

I don't have a strong opinion about DOM nodes. AFAIK, almost all of RefCounted classes are unable to copy, and we may want to have operator== implementations with pointer comparison.

--
TAMURA Kent
Software Engineer, Google

[Paweł Hajdan, Jr.]

Note that Reference& (and not const Reference&) obscures in the call sites that the object can actually be changed. This is more obvious with pointers which require the address to be &taken or the variable type to be a pointer.

Paweł

[Peter Kasting]

On Mon, Sep 9, 2013 at 6:10 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Note that Reference& (and not const Reference&) obscures in the call sites that the object can actually be changed. This is more obvious with pointers which require the address to be &taken or the variable type to be a pointer.

For this reason the Google (and hence Chromium) Style Guide bans non-const reference arguments to functions, which is a practice I suggest the Blink community adopt if it hasn't already done so.

(In general, I normally go further and suggest in my code reviews that people avoid non-const refs entirely, using either const refs or pointers as appropriate.)

PK 

[Elliott Sprehn]

If document() returns a Document& then you'd need to convert it back into a pointer to pass it to anything (or we switch it back to Document*). I think the Document* might be better, but it does mean we end up with null checks when developers don't understand the lifetime of objects.

I had suggested a NotNull<T> template that doesn't have a boolean conversion operator but otherwise acts like a regular ptr to prevent that but still have the normal ptr semantics. The idea a bit silly, but it would give us the same "this can never be null" documentation without the scariness that is non-const references.

- E

[Peter Kasting]

On Mon, Sep 9, 2013 at 6:37 PM, Elliott Sprehn <esp...@chromium.org> wrote:

On Mon, Sep 9, 2013 at 6:32 PM, Peter Kasting <pkas...@google.com> wrote:

On Mon, Sep 9, 2013 at 6:10 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Note that Reference& (and not const Reference&) obscures in the call sites that the object can actually be changed. This is more obvious with pointers which require the address to be &taken or the variable type to be a pointer.

For this reason the Google (and hence Chromium) Style Guide bans non-const reference arguments to functions, which is a practice I suggest the Blink community adopt if it hasn't already done so.

(In general, I normally go further and suggest in my code reviews that people avoid non-const refs entirely, using either const refs or pointers as appropriate.)

If document() returns a Document& then you'd need to convert it back into a pointer to pass it to anything (or we switch it back to Document*). I think the Document* might be better, but it does mean we end up with null checks when developers don't understand the lifetime of objects.

Yes.

One thing WebKit has not historically done (at least in code I ever saw; disclaimer, I haven't seen very much) is to extensively use debug assertions (in Chromium land, DCHECKs) as a way of documenting function pre- and post-conditions, as well as explicitly commenting on all functions taking and returning pointers what the lifetime, ownership model, possibility of being NULL, etc. are.

I don't know why so much code in the renderer seems to assume that commenting about such things is a bad thing and that it's much better for huge swaths of classes and call chains to just take or return raw pointers with no indication of their underlying constraints or assumptions.  Something like "this can't be NULL here" is a critical part of understanding a piece of code, and knowing whether, when you see that violated, it means you should just add a NULL-check, or back up to look for a bug in the previous code that violated a postcondition.

PK

[Christophe Dumez]

Hi,

Sorry I started working on this without discussing it on the mailing list first. I received some positive feedback from OWNERS on a bug [1] so I went ahead.

I realise that the mailing list is a better place to discuss this though, and I am happy to discuss it here.

On Tue, Sep 10, 2013 at 12:39 AM, Adam Klein <ad...@chromium.org> wrote:

I've seen a lot of changes going by over the last several weeks
changing, e.g., Node::document() now returns a Document& instead of
Document*. My understanding was that these are being ported over from
similar work in WebKit, but I was surprised that there wasn't a

The approach is the same as WebKit, yes. Most patches are not ported from WebKit though (with the exception of the Ref class which did not land).

 

discussion about this change on blink-dev, especially given that I've
seen comments on code reviews from some Blink OWNERS (at least esprehn
and eseidel, CCed) that they're not sure the current approach is
ideal.

To be fair, the objections were mostly about the Ref class, not really about using references. As a matter of fact, I have seen esprehn update code to use references as well. The problem is that the Ref class seems to have limited use and makes the code less readable in some cases. As a consequence, this patch did not land.

 

So, consider this a place to have that discussion. I'd hope that
besides broadening knowledge and consensus about the right way of
making this change in the codebase, this thread would also end up
giving us a few sentences to put in the style guide governing when to
use pointers vs references.

 I am personally in favor of using references as it avoids useless null checks and it is self documenting. There is also not the overhead of introducing a new pointer types (e.g. NotNullPtr<T>).

 I heard the following criticism:

- If you return a reference then you will need to convert it back to a pointer to pass it to a method. This is true for now because the work is in early stages. However, I believe a lot of methods that take a pointer argument actually expect it to be non-null and therefore could be converted to references as well. When I initially made Node::document() return a reference, I had to convert it most of the time into a pointer at some point in call sites. In follow-up patches, I started updating method arguments to use Document references instead of pointers when possible. As a result, we need to convert it back to a pointer less and less.

- Using a non-const reference obscures in the call sites that the object can actually be changed. This is a valid point. We need to be careful when given a reference that you can change the state of that object. The good news is that a lot of types (especially those allocated on the heap and passed around, including RefCounted objects) are not copyable. So at least you cannot usually assign to the reference. For copyable types, we either need to be extra careful or not use references.

The alternatives seem to be to document internal API to clarify lifetime of argument / return value and use more assertions. This would definitely be an improvement over the current situation (without references). This is a bit more work to us as we need to document more and this is not really a habit of (former-)WebKit developers. This is also not fool-proof as a developer may not read the comments are still do a NULL-check. One reason we did not like comments in WebKit is that they easily get outdated and they have a maintenance cost. The worst thing that could happen is that someone updates a method in a way that makes it return NULL sometimes and forgets to update the method's documentation to state so. In which case, other developers are likely to not do null-checks because of the outdated comment, which would be wrong.

Using references is self-documenting, thus has no maintenance cost and seems more fool proof when it comes to null-checks.

Kr,

Chris.

[1] https://code.google.com/p/chromium/issues/detail?id=281400

[Darin Fisher]

How does the OilPan work impact this? That work is happening on a branch, but in a nut-shell it is about changing reference counted DOM nodes into heap managed objects that can be GC'd. It involves using Handle<T> in place of raw pointers. Will all of the work to change pointers to references just be wasted effort if we have to again change them to Handle<T>?

-Darin

[Mike Lawther]

On 10 September 2013 11:32, Peter Kasting <pkas...@google.com> wrote:

On Mon, Sep 9, 2013 at 6:10 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Note that Reference& (and not const Reference&) obscures in the call sites that the object can actually be changed. This is more obvious with pointers which require the address to be &taken or the variable type to be a pointer.

For this reason the Google (and hence Chromium) Style Guide bans non-const reference arguments to functions, which is a practice I suggest the Blink community adopt if it hasn't already done so.

+1 

[Kentaro Hara]

How does the OilPan work impact this? That work is happening on a branch, but in a nut-shell it is about changing reference counted DOM nodes into heap managed objects that can be GC'd. It involves using Handle<T> in place of raw pointers. Will all of the work to change pointers to references just be wasted effort if we have to again change them to Handle<T>?

Even with Oilpan, it won't be a waste.

If we have:

  void foo() {

    Document* doc = bar();

    if (doc) { doc->baz(); }

  }

then we will rewrite the code to:

  void foo() {

    Handle<Document> doc = bar();

    if (doc) { doc->baz(); }

  }

On the other hand, if we have:

  void foo() {

    Document doc = bar();  // returns a reference.

    doc.baz();

  }

then we will rewrite the code to:

  void foo() {

    Handle<Document> doc = bar();

    doc->baz();

  }

This code has no if branch, so this is better :)

--
Kentaro Hara, Tokyo, Japan

[Eric Seidel]

On Mon, Sep 9, 2013 at 11:41 PM, Kentaro Hara <har...@chromium.org> wrote:
>> How does the OilPan work impact this? That work is happening on a branch,
>> but in a nut-shell it is about changing reference counted DOM nodes into
>> heap managed objects that can be GC'd. It involves using Handle<T> in place
>> of raw pointers. Will all of the work to change pointers to references just
>> be wasted effort if we have to again change them to Handle<T>?
>
>

> Even with Oilpan, it won't be a waste.
>
> If we have:
>
> void foo() {
> Document* doc = bar();
> if (doc) { doc->baz(); }
> }
>
> then we will rewrite the code to:
>
> void foo() {
> Handle<Document> doc = bar();
> if (doc) { doc->baz(); }
> }
>
> On the other hand, if we have:
>
> void foo() {
> Document doc = bar(); // returns a reference.
> doc.baz();
> }
>
> then we will rewrite the code to:
>
> void foo() {
> Handle<Document> doc = bar();
> doc->baz();
> }
>
> This code has no if branch, so this is better :)

This seems to support the idea of having a NotNull<T> type, presumably
with oilpan we might want a NotNullHandle<T>?

[Kentaro Hara]

This seems to support the idea of having a NotNull<T> type, presumably
with oilpan we might want a NotNullHandle<T>?

NotNullHandle<T> sounds like a good idea to me.

- If Blink replaces non-null T* to T&, then Oilpan will rewrite it to NotNullHandle<T>.

- If Blink replaces non-null T* to NotNull<T>, then Oilpan will rewrite it to NotNullHandle<T>.

So, from the perspective of Oilpan, once we completely move to the Oilpan world, the result might be the same.

[Darin Fisher]

Good point. I guess a secondary factor is that this work will increase merge conflicts, but you are probably swimming in those for other reasons anyways.

-Darin

[Mads Sig Ager]

I agree. I think these changes are fairly independent from oilpan. We will have to add a couple of NonNull pointer types (probably ResultNonNull and HandleNonNull) but that is certainly doable.

[TAMURA, Kent]

I made an experimental CL to add WTF::NonNullPtr<T>.

https://codereview.chromium.org/23890025/

Is this better than references?

[Christophe Dumez]

Hi,

Thanks tkent@!

I personally like it:

- Useless null checks will (still) not compile

- Less refactoring work as it can still be used as a pointer (unlike references), we only need to update the APIs, not the code.

- Makes pointer comparison nice again. With references we had to do (&a == &b)

- Could potentially have NonNullRefPtr / NonNullPassRefPtr alternatives?

One drawback I can see is that it makes it less obvious (compared to references) to reviewers when null checks are missing. The code still looks unsafe as the type still has pointer semantics:

a.b().c() looks safe, but a->b()->c() does not. As a reviewer, you would have to look at the declarations of a() and b() to make sure this is correct (i.e. they return NonNullPtr<T> types).

Kr,

Christophe Dumez.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Ojan Vafai]

I don't really see the problem with non-const references. I don't feel strongly either way. I'd like for us to adopt one of these solutions though. I think it's a good change to have more of the code statically declare that a value can't be null.

[David Levin]

On Fri, Sep 13, 2013 at 4:04 AM, Christophe Dumez <ch.d...@sisa.samsung.com> wrote:

Hi,

Thanks tkent@!

I personally like it:

- Useless null checks will (still) not compile

- Less refactoring work as it can still be used as a pointer (unlike references), we only need to update the APIs, not the code.

- Makes pointer comparison nice again. With references we had to do (&a == &b)

- Could potentially have NonNullRefPtr / NonNullPassRefPtr alternatives?

One drawback I can see is that it makes it less obvious (compared to references) to reviewers when null checks are missing. The code still looks unsafe as the type still has pointer semantics:

a.b().c() looks safe, but a->b()->c() does not. As a reviewer, you would have to look at the declarations of a() and b() to make sure this is correct (i.e. they return NonNullPtr<T> types).

One could have a compile assert which uses a type trait to document that "a" etc are of type NonNullPtr<T> instead of the null check. You have to see if this became too ugly or not.

[Yuta Kitamura]

On Tue, Sep 10, 2013 at 8:33 AM, TAMURA, Kent <tk...@chromium.org> wrote:

I'm sorry that I started some work without any notification.

I converted some arguments and return values in Page and the editing area. I found no downsides in these areas because

 - Ownership is very clear.  e.g. Page owns a FocusController.

 - No need to compare objects.

I removed a lot of unnecessary NULL checks, and it must have reduced binary size and source code size, and we don't need to worry about NULL deferences of these objects any longer.

I think it's worth to proceed the reference-conversion for objects handled by OwnPtr or similar relationship.

The difference between the above case and DOM nodes:

 - DOM node values are never null in some cases.  e.g. Node::document() and Document argument of Node factories

 - DOM node values can be null in some cases.  e.g. Document::focusedElement() can be null.

 - We need to compare DOM nodes frequently.

I don't have a strong opinion about DOM nodes. AFAIK, almost all of RefCounted classes are unable to copy, and we may want to have operator== implementations with pointer comparison.

As far as I know, our IDL code generator is ignorant of non-nullable interface type (T) in function arguments, and treats such arguments as nullable ones (T?). And generated code passes a pointer (T*) to core classes. I think this (assumingly historical) behavior causes a lot of confusion regarding nullable/non-nullable pointers.

From the WebIDL spec's point of view, our current behavior is wrong (if null is passed to an argument of a non-nullable interface type, a browser should throw TypeError instead of DOMException, and methods of core classes should not be called at all).

So, in my view, we should consciously distinguish non-nullable interface arguments (T) and nullable ones (T?), and arguments in the core classes should reflect the difference (T& / T*, or NonNull<T> / T* or whatever). Of course our IDL generator should be able to understand this scheme. We might not be able to implement this change very quickly but I think we should aim for it as a mid- or long-term goal.

Thanks,

Yuta

[TAMURA, Kent]

I guess the following rules will satisfy many developers.

 * Use NonNullPtr<T>, not C++ references

   It's compatible with Chromium coding style.

   (We should have shorter better name.  NotNull<T>?)

 * Don't add Ref<T>, which is non-nullable RefPtr<T>.  Don't support Non-nullable types in IDL.

   They have additional complexity, and need too much code change.  We should revisit them when we have the decision about Oilpan.

WDYT?

[Ojan Vafai]

On Mon, Sep 23, 2013 at 8:38 AM, TAMURA, Kent <tk...@chromium.org> wrote:

I guess the following rules will satisfy many developers.

 * Use NonNullPtr<T>, not C++ references

   It's compatible with Chromium coding style.

   (We should have shorter better name.  NotNull<T>?)

I prefer the shorter name.

 

 * Don't add Ref<T>, which is non-nullable RefPtr<T>.  Don't support Non-nullable types in IDL.

   They have additional complexity, and need too much code change.  We should revisit them when we have the decision about Oilpan.

WDYT?

SGTM. 

[Christophe Dumez]

Sounds good to me as well, Thanks.

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

--
Christophe DUMEZ

[Adam Klein]

I discussed this in person with some folks yesterday who shared my concerns and so decided to chime in on the thread.

I don't think we should re-invent non-nullity (not least because NotNull<Document> everywhere looks very verbose to me). If we want to have non-null references, we should use our language's native version: C++ references.

I'll address each complaint I've heard about that approach.

Problem: Comparisons between, e.g., Document& now need "&" to compare pointers

Solution: Add operator== for Nodes (or maybe just TreeShared) that compares pointers.

Problem: Chromium style says not to use non-null references, and to use pointers instead

Solution: Blink style has long used references for out-params, and elsewhere (e.g., for some DEFINE_STATIC_LOCALs). Blink style needn't be identical to Chromium style, especially when we're already following existing Blink style practices.

Problem: WebKit's Ref class is weird

Solution: Don't use it. If we want to reduce the use of the & operator, we could add a RefPtr(T&) constructor.

If folks are interested in discussing further, I propose we have a 15-minute huddle sometime today as part of BlinkOn.

- Adam

[Hajime Morrita]

I'd support this with some bike-shedding:

On Wed, Sep 25, 2013 at 11:27 AM, Adam Klein <ad...@chromium.org> wrote:

I discussed this in person with some folks yesterday who shared my concerns and so decided to chime in on the thread.

I don't think we should re-invent non-nullity (not least because NotNull<Document> everywhere looks very verbose to me). If we want to have non-null references, we should use our language's native version: C++ references.

I'll address each complaint I've heard about that approach.

Problem: Comparisons between, e.g., Document& now need "&" to compare pointers

Solution: Add operator== for Nodes (or maybe just TreeShared) that compares pointers.

+1. If we feel it confusing, we could add a named member function like Node::is(const Node&).

Problem: Chromium style says not to use non-null references, and to use pointers instead

Solution: Blink style has long used references for out-params, and elsewhere (e.g., for some DEFINE_STATIC_LOCALs). Blink style needn't be identical to Chromium style, especially when we're already following existing Blink style practices.

Problem: WebKit's Ref class is weird

Solution: Don't use it. If we want to reduce the use of the & operator, we could add a RefPtr(T&) constructor.

+1 as well. We could also have RefPtr<T> TreeShared<T>::toPtr() if we want make it explicit.

Code brevity is one of the great assets of Blink, especially compared to other aged C++ codebases.

It'd be great if we don't hurt it.

If folks are interested in discussing further, I propose we have a 15-minute huddle sometime today as part of BlinkOn.

- Adam

On Tue, Sep 24, 2013 at 4:05 AM, Christophe Dumez <dch...@gmail.com> wrote:

Sounds good to me as well, Thanks.

On Mon, Sep 23, 2013 at 6:38 PM, TAMURA, Kent <tk...@chromium.org> wrote:

I guess the following rules will satisfy many developers.

 * Use NonNullPtr<T>, not C++ references

   It's compatible with Chromium coding style.

   (We should have shorter better name.  NotNull<T>?)

 * Don't add Ref<T>, which is non-nullable RefPtr<T>.  Don't support Non-nullable types in IDL.

   They have additional complexity, and need too much code change.  We should revisit them when we have the decision about Oilpan.

WDYT?

--
TAMURA Kent
Software Engineer, Google

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

--
Christophe DUMEZ

--
morrita

[TAMURA, Kent]

Yeah, I actually prefer references. I compromised for Chromium-style-lovers in my last proposal.

I'll make a voting form. If the number of votes for a choice is much larger than others, let's adopt it.

[Aaron Boodman]

This leaves out the main argument against non-const refs, the underlying rationale for Chrome's styleguide:

Problem: With non-const refs, you can't tell at the callsite whether a function will modify an input parameter.

Also, a bit of a nit, but C++ references are not "non-null". They are perfectly happy to be null:

Foo *f = NULL;

Foo& f2 = *f;

printf("%d", f2.bonk);

You can imagine this will happen all the time accidentally when converting to reference params or return values.

At best, all mutable references are is a stylistic convention that says such values are not expected to be NULL. It will still be up to the calling code to manually maintain this expectation.

The advantage of NonNull<T> is that you could actually put a DCHECK in there that enforces the invariant. 

- a

[Peter Kasting]

On Thu, Sep 26, 2013 at 1:19 PM, Aaron Boodman <a...@chromium.org> wrote:

Problem: With non-const refs, you can't tell at the callsite whether a function will modify an input parameter.

Also, a bit of a nit, but C++ references are not "non-null".

The advantage of NonNull<T> is that you could actually put a DCHECK in there that enforces the invariant. 

+2

PK 

[Elliott Sprehn]

Yeah, I like it for that reason as well.

- E 

[Nico Weber]

On Thu, Sep 26, 2013 at 1:19 PM, Aaron Boodman <a...@chromium.org> wrote:

This leaves out the main argument against non-const refs, the underlying rationale for Chrome's styleguide:

Problem: With non-const refs, you can't tell at the callsite whether a function will modify an input parameter.

Also, a bit of a nit, but C++ references are not "non-null". They are perfectly happy to be null:

Foo *f = NULL;

Foo& f2 = *f;

This is undefined behavior (8.3.2p4) and can be found by asan or valgrind just as well as the DCHECK as far as I know ( http://valgrind.org/docs/origin-tracking2007.pdf ).

[James Robinson]

On Thu, Sep 26, 2013 at 1:19 PM, Aaron Boodman <a...@chromium.org> wrote:

This leaves out the main argument against non-const refs, the underlying rationale for Chrome's styleguide:

Problem: With non-const refs, you can't tell at the callsite whether a function will modify an input parameter.

Also, a bit of a nit, but C++ references are not "non-null". They are perfectly happy to be null:

No, there is no such things as a C++ reference to null.  Any attempt to construct one must invoke undefined behavior which could do anything.

Foo *f = NULL;

Foo& f2 = *f;

This line exhibits undefined behavior.  It could do literally anything and it's impossible to check for after the fact.  I.e. if you were to try to then say

Foo* f3 = &f2;

if (!f3)

  CRASH();

f3->foobar();

A compile would be perfectly within its rights to optimize the check out completely and crash on the foobar() invocation.

 

printf("%d", f2.bonk);

This line could do anything.

- James

[James Robinson]

Even more fun can be had!  If you write:

void blah(Foo* foo) {

  Foo& r = *foo; // 1

  if (foo == NULL)  // 2

    return;

  foo->blahblah();  // 3

}

Then you can get a NULL crash on 3.  How?  The compiler can optimize out the null check in 2 since if foo actually was NULL line 1 exhibited undefined behavior and thus the compiler is free to do whatever it wants.  This is even if 'r' is never used.

Whether the compiler actually does this transformation depends on lots and lots of things, but compiler authors are very clever and know the standards better than you and a very happy to break standards-incompatible code.

- James

[Aaron Boodman]

On Thu, Sep 26, 2013 at 1:28 PM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Sep 26, 2013 at 1:19 PM, Aaron Boodman <a...@chromium.org> wrote:

This leaves out the main argument against non-const refs, the underlying rationale for Chrome's styleguide:

Problem: With non-const refs, you can't tell at the callsite whether a function will modify an input parameter.

Also, a bit of a nit, but C++ references are not "non-null". They are perfectly happy to be null:

Foo *f = NULL;

Foo& f2 = *f;

This is undefined behavior (8.3.2p4)

It doesn't seem like the undefinedness helps us in practice -- it's still a very easy bug to type accidentally.

 

and can be found by asan or valgrind just as well as the DCHECK as far as I know ( http://valgrind.org/docs/origin-tracking2007.pdf ).

Perhaps. I guess my take is that the same way a bug found by the compiler is preferrable to a bug found by a unit test, a bug found by a DCHECK is better than a bug found by valgrind.

- a

[Nico Weber]

On Thu, Sep 26, 2013 at 1:35 PM, Aaron Boodman <a...@chromium.org> wrote:

On Thu, Sep 26, 2013 at 1:28 PM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Sep 26, 2013 at 1:19 PM, Aaron Boodman <a...@chromium.org> wrote:

This leaves out the main argument against non-const refs, the underlying rationale for Chrome's styleguide:

Problem: With non-const refs, you can't tell at the callsite whether a function will modify an input parameter.

Also, a bit of a nit, but C++ references are not "non-null". They are perfectly happy to be null:

Foo *f = NULL;

Foo& f2 = *f;

This is undefined behavior (8.3.2p4)

It doesn't seem like the undefinedness helps us in practice -- it's still a very easy bug to type accidentally.

 

and can be found by asan or valgrind just as well as the DCHECK as far as I know ( http://valgrind.org/docs/origin-tracking2007.pdf ).

Perhaps. I guess my take is that the same way a bug found by the compiler is preferable to a bug found by a unit test

(I agree with this part.)

 

, a bug found by a DCHECK is better than a bug found by valgrind.

Why is that? The DCHECK doesn't find it any earlier, it just finds it with a better stack – but only if it's a NULL, not an invalid address elsewhere, while valgrind will give you a good stack in both cases.

 

- a

[Aaron Boodman]

Time from mistake to symptom. With the DCHECK you have a good chance of stumbling across the bug while developing locally normally. With valgrind you either have to test with valgrind locally (unlikely) or wait for it to get commited and run on the bots.

I also feel like the project has a bad habit of supressing valgrind problems, but maybe that's gotten better.

- a

[dongseo...@intel.com]

How about taking both benefits?

For example,

class Frame {

   Document& document();

   NotNull<Document> m_document;

}

The owner should use NotNull<T>, and return type and argument use C++ reference.

Br, DS

[TAMURA, Kent]

We don't have a silver bullet. Any method has downsides.

On Fri, Sep 27, 2013 at 5:19 AM, Aaron Boodman <a...@chromium.org> wrote:

This leaves out the main argument against non-const refs, the underlying rationale for Chrome's styleguide:

Problem: With non-const refs, you can't tell at the callsite whether a function will modify an input parameter.

Right. It's a downside of references.  On the other hand, we don't need to worry about nullness of a parameter if we adopt references.

When we have a function with a pointer parameter,

 - Function author needs to define a behavior with NULL parameter, document it, and assert it.

 - Function users need to investigate whether the function accepts NULL or not.

 

Also, a bit of a nit, but C++ references are not "non-null". They are perfectly happy to be null:

Foo *f = NULL;

Foo& f2 = *f;

printf("%d", f2.bonk);

You can imagine this will happen all the time accidentally when converting to reference params or return values.

We had better introduce a conversion function?

template <typename T> T& dereference(T* ptr)

{

    ASSERT(ptr);

    return *ptr;

}

Foo& f2 = dereference(f);

[Thomas Sepez]

I'm late to the party but I'm in favor of anything except introducing new templates.  The further away something is from what it actually is, the harder it is to see.

[TAMURA, Kent]

Result: 62% of voters prefer references.

https://docs.google.com/a/chromium.org/forms/d/1f1QSzGOzAWrG4zuqy9JdJGhgZp0ZV3-MzMJfcRPu_tE/viewanalytics

Note: 76% of core owners prefer references.

So, I'd like to conclude we should use references to represent non-null objects.

It doesn't mean we use references forever. If we decided to follow Chromium coding style entirely or found critical problems with references, we would switch back to pointers or NotNull<T>. Even if the switching back happens, using references for now is beneficial because it clarifies what are not nullable.

[Peter Kasting]

On Thu, Oct 3, 2013 at 7:39 AM, TAMURA, Kent <tk...@chromium.org> wrote:

So, I'd like to conclude we should use references to represent non-null objects.

It doesn't mean we use references forever. If we decided to follow Chromium coding style entirely or found critical problems with references, we would switch back to pointers or NotNull<T>. Even if the switching back happens, using references for now is beneficial because it clarifies what are not nullable.

This is not the kind of switch we should be making multiple times.  If we switch, it should be because we intend to change forever.

There have been stated (if vague) goals in the past to align more with Google and Chromium style over the long term.  This would be a pretty strong move in the opposite direction.  Rather than just taking a poll about what people would like in the abstract, I'd like to explicitly hear the other owners' comments on that particular issue, as well as the underlying reason for the Google style rule -- which Aaron originally raised, but got lost in discussion about whether references can actually be NULL.  No one addressed the much more critical issue of why the Google (and thus Chromium) style guides ban non-const ref arguments in the first place, which is clarity at the callsites.

PK

[Adam Klein]

On Thu, Oct 3, 2013 at 10:45 AM, Peter Kasting <pkas...@google.com> wrote:

On Thu, Oct 3, 2013 at 7:39 AM, TAMURA, Kent <tk...@chromium.org> wrote:

So, I'd like to conclude we should use references to represent non-null objects.

It doesn't mean we use references forever. If we decided to follow Chromium coding style entirely or found critical problems with references, we would switch back to pointers or NotNull<T>. Even if the switching back happens, using references for now is beneficial because it clarifies what are not nullable.

This is not the kind of switch we should be making multiple times.  If we switch, it should be because we intend to change forever.

What's your argument for these assertions? Given that Node::document() already returns a reference, using these as our axioms would mean that the future's already set, and so we could stop this thread altogether.

 

There have been stated (if vague) goals in the past to align more with Google and Chromium style over the long term.  This would be a pretty strong move in the opposite direction.

 A quick 'git grep' in Blink's Source directory reveals at least ~3000 lines already using non-const references (my simple pattern only matches functions whose first argument takes a reference; there are definitely many places where it's a second or third argument, or a return value). A change requiring using pointers would be a huge change to Blink style already, whether we use references in more places or not.

 

Rather than just taking a poll about what people would like in the abstract, I'd like to explicitly hear the other owners' comments on that particular issue, as well as the underlying reason for the Google style rule -- which Aaron originally raised, but got lost in discussion about whether references can actually be NULL.

Concretely, the reason I (as a Blink owner) would prefer references over pointers are that they make it clear that the value cannot be null (I think I already stated this up thread). This is not a side-point, it's in fact the whole point of this thread (and the changes it refers to).

 

No one addressed the much more critical issue of why the Google (and thus Chromium) style guides ban non-const ref arguments in the first place, which is clarity at the callsites.

I don't see that it's relevant why Google/Chromium style bans non-const references, given that Blink encourages them (http://www.chromium.org/blink/coding-style#TOC-Pointers-and-References) and, as noted above, Blink code also follows this pattern throughout. If you wish to change Blink style to outlaw non-const references, I recommend starting that discussion in a different thread.

- Adam

[Peter Kasting]

On Thu, Oct 3, 2013 at 11:13 AM, Adam Klein <ad...@chromium.org> wrote:

Given that Node::document() already returns a reference, using these as our axioms would mean that the future's already set, and so we could stop this thread altogether.

I'm confused.  I thought the entire point of this thread was that the current codebase uses pointers extensively, and the question was whether to shift to references in all places where the pointer cannot be NULL.  Your original message in the thread seems to imply that such a shift has just started to take effect, and that we should discuss it before it actually becomes widespread.

Now you seem to be saying that the codebase uses references extensively, and a decision to "use references" means leaving things as they are.  Indeed, you even tell me explicitly that an argument for _not_ using references in some cases is out of scope for this thread, which I find quite surprising.

Rather than just taking a poll about what people would like in the abstract, I'd like to explicitly hear the other owners' comments on that particular issue, as well as the underlying reason for the Google style rule -- which Aaron originally raised, but got lost in discussion about whether references can actually be NULL.

Concretely, the reason I (as a Blink owner) would prefer references over pointers are that they make it clear that the value cannot be null (I think I already stated this up thread). This is not a side-point, it's in fact the whole point of this thread (and the changes it refers to).

Perhaps I was unclear.  I understand already that the benefit of references is as a way to indicate "cannot be NULL".  I wasn't talking about whether indicating such a thing was a good idea.  I was asking explicitly for comments on the issue of callsite unclarity due to non-const refs, as that is a restriction on code outside Blink, and lots of important folks have expressed desires in the past to migrate Blink and Chromium styles closer together over time.

The reason this is relevant is because if we intend to change the Blink style guide to require references in all cases where arguments cannot be NULL, that implicitly requires non-const refs any time those arguments aren't passed by value; and so we should either knowingly decide that we want that, and we disagree with (or don't care about) the Google style guide, or make some sort of exception/alternative plan.

PK

[Adam Barth]

You two seem to be talking past each other a bit.  There are two separate issues:

1) Should we use non-const references to represent non-NULL pointers to non-copyable object (e.g., Document).

2) Should we use non-const references as out parameters for copyable types (e.g., Vector).

It might be worth discussing these topics in separate threads as they are largely unrelated to each other.  For example, whether a function receives a Document* or a Document& had no effect on what changes that function can make to the Document or the caller's local variables whereas if we pass a Vector<Foo>& to a function, then the function is able to modify local variables in the caller's scope.

Adam

[Peter Kasting]

On Thu, Oct 3, 2013 at 12:09 PM, Adam Barth <aba...@chromium.org> wrote:

You two seem to be talking past each other a bit.  There are two separate issues:

1) Should we use non-const references to represent non-NULL pointers to non-copyable object (e.g., Document).

2) Should we use non-const references as out parameters for copyable types (e.g., Vector).

It might be worth discussing these topics in separate threads as they are largely unrelated to each other.  For example, whether a function receives a Document* or a Document& had no effect on what changes that function can make to the Document or the caller's local variables whereas if we pass a Vector<Foo>& to a function, then the function is able to modify local variables in the caller's scope.

I don't know if I followed that 100%.  If I have void foo(Document* doc), It's clear that foo() can modify |doc| regardless of whether |doc| is copyable.  Changing to a non-const ref means the function can still modify its arg, still without regard to whether the object is copyable.  It doesn't seem like object copyability factors into things?

PK

[Alec Flett]

Its the difference between being able to do this:

void foo(Document* doc) {

    *doc = Document();

}

and not being able to:

void foo(Document& foo) {

    foo = Document() // this just changes the local ref.

}

Alec

 

PK

[Adam Barth]

On Thu, Oct 3, 2013 at 1:23 PM, Peter Kasting <pkas...@google.com> wrote:

void foo(...) {

  Document* doc = ...

  bar(doc);

  baz(*doc);

}

void bar(Document*) { ... }

void baz(Document&) { ... }

I believe the effects that bar and baz can have on foo and doc are the same.  That's issue (1).  Issue (2) is about the following pattern:

void foo(...) {

  int x = 0;

  bar(x);

}

void bar(int& result) { ... }

In this case, it's not clear from the text of |foo| what effects calling bar will have on x.  The bar function might alter the value of x unexpectedly.

Adam

[Peter Kasting]

On Thu, Oct 3, 2013 at 1:28 PM, Alec Flett <alec...@chromium.org> wrote:

Its the difference between being able to do this:

void foo(Document* doc) {

    *doc = Document();

}

and not being able to:

void foo(Document& foo) {

    foo = Document() // this just changes the local ref.

}

Unless I'm wildly misunderstanding, your code doesn't compile.

C++ does not allow you to rebind references.  If Document has inaccessible copy/assignment operators, "foo = Document()" will barf saying that Document::operator=() is private.  It won't rebind |foo| locally to point at a different object, leaving the caller seeing an unchanged object.  (I just test-compiled this code to verify this.)

Whereas, if Document::operator=() is public, this code compiles, but |foo|'s contents are changed to those of an empty document, and that change is visible caller-side.

PK

[Peter Kasting]

On Thu, Oct 3, 2013 at 1:39 PM, Adam Barth <aba...@chromium.org> wrote:

void foo(...) {

  Document* doc = ...

  bar(doc);

  baz(*doc);

}

void bar(Document*) { ... }

void baz(Document&) { ... }

I believe the effects that bar and baz can have on foo and doc are the same.  That's issue (1).  Issue (2) is about the following pattern:

void foo(...) {

  int x = 0;

  bar(x);

}

void bar(int& result) { ... }

In this case, it's not clear from the text of |foo| what effects calling bar will have on x.  The bar function might alter the value of x unexpectedly.

But you don't know, when writing the called function, what the context of the caller will be.  After all:

void caller1() {

  Document* doc = ...;

  callee(*doc);

}

void caller2() {

  Document doc(...);

  callee(doc);

}

void callee(Document& doc) { ... }

(a) When writing callee(), you don't know whether the caller looks like caller1() or caller2().

(b) Even if you did know, I claim that modification of |doc| at the caller-side is equally surprising in both functions, and distinct from the surprisingness of calling through a pointer arg.

PK

[Adam Barth]

It's not possible to write caller2.  Document's constructor is private and, even if you were inside Document, the code will ASSERT.  Documents must be created via the Document::create static function.

Adam

[Adam Klein]

On Thu, Oct 3, 2013 at 11:25 AM, Peter Kasting <pkas...@google.com> wrote:

On Thu, Oct 3, 2013 at 11:13 AM, Adam Klein <ad...@chromium.org> wrote:

Given that Node::document() already returns a reference, using these as our axioms would mean that the future's already set, and so we could stop this thread altogether.

I'm confused.  I thought the entire point of this thread was that the current codebase uses pointers extensively, and the question was whether to shift to references in all places where the pointer cannot be NULL.  Your original message in the thread seems to imply that such a shift has just started to take effect, and that we should discuss it before it actually becomes widespread.

Now you seem to be saying that the codebase uses references extensively, and a decision to "use references" means leaving things as they are.  Indeed, you even tell me explicitly that an argument for _not_ using references in some cases is out of scope for this thread, which I find quite surprising.

I see that my framing of this discussion at the start of the thread was confusing, and for that I apologize. I did not mean to bring up the question of whether Blink should stop using references for "out" arguments, or for other places where they're used as return values or function arguments. Rather, I meant to address the specific cases where historically WebKit used pointers (mostly Node*, Element*, and Document*) and that have begun to shift towards references (most notably, Node::document() returning a Document&).

The complaints I heard most from Blink owners about the latter had nothing to do with the Google/Chromium style, but rather the syntactic awkwardness of using references where previously pointers were used (e.g., comparing via == now required taking the address of Node::document()) or with the danger of mechanical replacement of pointers with references causing accidental re-assignment (now being discussed down-thread).

Rather than just taking a poll about what people would like in the abstract, I'd like to explicitly hear the other owners' comments on that particular issue, as well as the underlying reason for the Google style rule -- which Aaron originally raised, but got lost in discussion about whether references can actually be NULL.

Concretely, the reason I (as a Blink owner) would prefer references over pointers are that they make it clear that the value cannot be null (I think I already stated this up thread). This is not a side-point, it's in fact the whole point of this thread (and the changes it refers to).

Perhaps I was unclear.  I understand already that the benefit of references is as a way to indicate "cannot be NULL".  I wasn't talking about whether indicating such a thing was a good idea.  I was asking explicitly for comments on the issue of callsite unclarity due to non-const refs, as that is a restriction on code outside Blink, and lots of important folks have expressed desires in the past to migrate Blink and Chromium styles closer together over time.

Thanks for the explanation. I hope I made clear above why I think the "callsite unclarity" issue should be separate from this discussion.

 

The reason this is relevant is because if we intend to change the Blink style guide to require references in all cases where arguments cannot be NULL, that implicitly requires non-const refs any time those arguments aren't passed by value; and so we should either knowingly decide that we want that, and we disagree with (or don't care about) the Google style guide, or make some sort of exception/alternative plan.

I'm not sure we would want to change the Blink style guide to _require_ non-null arguments to be passed by reference (especially since this would instantly make much of the code-base non-conforming). But given that the style guide already allows it, and that there are already over 3000 cases where this happens, it seems unlikely to me that using references in more places is going to make it significantly harder than it already would be to replace those references with pointers (leaving aside for the moment whether the Blink community would even wish to make that change to the style guide).

- Adam

[Peter Kasting]

OK, but this still leaves point (b), as well as:

void foo(Document& doc) {

  ...

  bar(doc);  // Can modify the contents of |doc|

}

Both these cases still seem potentially surprising, at least to me.

PK

[Adam Barth]

Isn't the set of things |bar| can do above the same as in the following code?

void foo(Document* doc) {

  ...

  bar(doc);  // Can modify the contents of |doc|

}

Adam

[Peter Kasting]

Yes, but if you see a bunch of:

  doc.func();

  doc.x = y;

  doc.func2();

  callee(doc);

vs.

  doc->func();

  doc->x = y;

  doc->func2();

  callee(doc);

Then by context, the second case looks like a pointer and thus probably modifiable, while the first doesn't.

Some of this might also relate to whether you have a global context that, in general, references are not modifiable.  If a codebase extensively uses non-const refs, perhaps someone working with the first case above is as likely to assume |doc| is modified in both cases.  I don't know.  I know that I would view these differently today.

PK

[TAMURA, Kent]

I guess many Blink developers don't agree with the concern. Many objects such as DOM nodes are really unstable, and developers always need to consider possibility that a function call modifies a passed object regardless of callee(doc) or callee(&doc).

Also, using pointers doesn't show such signals in many cases. For example,

The original code:

void caller() {

    Document* doc = document();

    ...

    callee(doc);

 }

Update for references:

void caller() {

    Document& doc = document();

    ...

    callee(doc);

 }

The line `callee(doc)' is not changed.

[Peter Kasting]

On Thu, Oct 3, 2013 at 2:06 PM, TAMURA, Kent <tk...@chromium.org> wrote:

I guess many Blink developers don't agree with the concern.

Well, determining that is why I wrote to begin with.  So far, not many of us have contributed, so I'm not prepared to generalize either way.

Many objects such as DOM nodes are really unstable, and developers always need to consider possibility that a function call modifies a passed object regardless of callee(doc) or callee(&doc).

void foo(const Document& doc);

Calling this should theoreticalyl never result in a modification of doc or any state visible (transitively) from doc.  Unfortunately it can in our current codebase, because we have a lot of physically-but-not-logically-const accessors that do things like returning non-const pointers from const methods:

Foo* Class::accessor() const;

I discussed this issue several years ago with Darin Adler, and made (and landed) a few inroads on cutting down such cases, but never got very far.

It would be valid to use this as an argument for why my concern is quixotic, because the codebase can already do practically anything at any time.  It would also be valid to suggest that perhaps we should move to reduce such cases.

I'm prepared for pretty much any outcome of this dicsussion.  I merely want to hear what the other folks with more expertise about Blink have to say.  If everybody else thinks there are no problems, I'm not going to gainsay that.

Also, using pointers doesn't show such signals in many cases.

I addressed this some in my latest reply to Adam (Barth).  I would also suggest that "there are cases where you don't get good signals" is not the same as saying "there are no cases where you _do_ get good signals".  The point of a style guide is to suggest global uses of certain patterns precisely so not everything comes down to a local determination.

PK

[Adam Klein]

On Thu, Oct 3, 2013 at 2:17 PM, Peter Kasting <pkas...@google.com> wrote:

On Thu, Oct 3, 2013 at 2:06 PM, TAMURA, Kent <tk...@chromium.org> wrote:

Many objects such as DOM nodes are really unstable, and developers always need to consider possibility that a function call modifies a passed object regardless of callee(doc) or callee(&doc).

void foo(const Document& doc); 

Some data from third_party/WebKit/Source:

$ git grep 'const Document[*&]' | wc -l

70

$ git grep 'Document[*&]' | wc -l 

2537

So this just doesn't come up very often, at least for Documents (numbers are similar for Node and Element, though there are more uses of const Node*).

- Adam

[Peter Kasting]

Yes.  One of the things that makes being more const-correct difficult is that most functions I've seen use "Class*" with no const-qualifiers, even when they might not theoretically need to.  Not surprising, since const's viral nature often means it's everywhere or nowhere, and in Blink, it's basically nowhere.

Anyway, I've almost certainly said more than I need to about all these subjects, so I'll attempt to stop replying at this point and leave the floor open :)

PK

[TAMURA, Kent]

I would be responsible for switching back, and would work on it by myself if we decided to follow Chromium coding style.

Fortunately Blink code is separated well from Chromium. This is a local issue and doesn't affect Chromium.

[Philip Jägenstedt]

This outcome pleases me. Since it was only yesterday that I found myself checking if Frame::tree() could return NULL I've prepared a patch to let Frame::loader(), navigationScheduler() and tree() return references. Can I request review for that right away, or does this issue need to be settled definitively first? It doesn't involve any out arguments, so ought to not be controversial.

Philip

[Eric Seidel]

Most instances where we store Frame* are likely (next week) to change to being FrameHandles for the out of process iframe work (which was discussed a couple weeks ago on blink-dev).

https://docs.google.com/a/chromium.org/document/d/1PeSpryRsXlLxHU58GOa0s67kuKaQeGitmM0iWS02CvU/edit

[Philip Jägenstedt]

Thanks Eric, I'll wait until that has landed to see if my patch is still relevant or not.

Philip

[TAMURA, Kent]

Also, using pointers doesn't show such signals in many cases.

I addressed this some in my latest reply to Adam (Barth).  I would also suggest that "there are cases where you don't get good signals" is not the same as saying "there are no cases where you _do_ get good signals".  The point of a style guide is to suggest global uses of certain patterns precisely so not everything comes down to a local determination.

You're right. Pointers for non-null objects still have benefit.

However the benefit is small in Blink, and the major part of voters thought non-const references had more benefit than pointers.