Why we allow comparison between a reference and a pointer?

[Xianzhu Wang]

Hi,

Just noticed that we can compare a reference to a pointer for some types (e.g. RenderObject). https://codereview.chromium.org/184023003/ etc. added the behavior.

The default language behavior of operator==(const T& a, const T& b) is also changed to comparing addresses.

Is this just temporary to avoid too much code change, or allowed (or even preferred?) in future code?

IMHO, the behaviors are bad because it's not the correct way C++ language should work.

Thanks,

Xianzhu

[Vyacheslav Ostapenko]

On other thought, it is used only for the types that didn't have comparison defined for them and cannot be compared by value (Node, RenderObject and Frame), so I don't see the problem how it used now, but definitely it is a potential error if DEFINE_COMPARSION_OPERATORS_WITH_REFERENCES is used incorrectly for some other types where comparison by value makes sense.

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

[Adam Klein]

On Tue, Jul 8, 2014 at 12:37 PM, Xianzhu Wang <wangx...@chromium.org> wrote:

Hi,

Just noticed that we can compare a reference to a pointer for some types (e.g. RenderObject). https://codereview.chromium.org/184023003/ etc. added the behavior.

The default language behavior of operator==(const T& a, const T& b) is also changed to comparing addresses.

 

Is this just temporary to avoid too much code change, or allowed (or even preferred?) in future code?

I believe I was the first to add such an operator, for Nodes, back when code was being converted to use references in more places. Since the only meaning of equality for Nodes was their address, it seemed semantically correct to me, besides making the reference-conversion patches smaller and making the resulting code "cleaner" (in the sense of having fewer '&' characters). 

IMHO, the behaviors are bad because it's not the correct way C++ language should work.

Can you expand on this argument (perhaps give an example where this operator is causing problems)? For all of the types where this has been done in Blink (to my knowledge), there is no other sense of equality than pointer equality. And so it seems perfectly within the realm of C++ to define operator== to test equality in an application-specific way.

- Adam

[Peter Kasting]

On Tue, Jul 8, 2014 at 3:34 PM, Adam Klein <ad...@chromium.org> wrote:

IMHO, the behaviors are bad because it's not the correct way C++ language should work.

Can you expand on this argument (perhaps give an example where this operator is causing problems)? For all of the types where this has been done in Blink (to my knowledge), there is no other sense of equality than pointer equality. And so it seems perfectly within the realm of C++ to define operator== to test equality in an application-specific way.

I tend to agree that this kind of overload is confusing.

I think the Google style guide would suggest that, instead of using an overloaded operator==() here, using an explicitly-named method (e.g. ptrEquals()) would be clearer and safer.

PK 

[Adam Klein]

The "cleaner" argument sounds dubious to me. Compare:

  if (node.document().ptrEquals(otherNode.document()))

with

  if (node.document() == otherNode.document())

Can you supply an example where ptrEquals is safer?

- Adam

[Peter Kasting]

I said clearer, not cleaner.  I definitely think the ptrEquals() call is clearer, functionally, than the use of == in your example above.  (isPtrEquivalent() would probably be an even better name.)

 

Can you supply an example where ptrEquals is safer?

It's safer in any case where a reader could imagine (accurately or not) objects being compared by value.  How likely that is depends on a reader's familiarity with the code and types in question.  I tend to want to guarantee safety for people who are at a lower familiarity level, since they're the most likely to introduce bugs.

PK

[Adam Klein]

On Tue, Jul 8, 2014 at 3:51 PM, Peter Kasting <pkas...@google.com> wrote:

On Tue, Jul 8, 2014 at 3:43 PM, Adam Klein <ad...@chromium.org> wrote:

On Tue, Jul 8, 2014 at 3:40 PM, Peter Kasting <pkas...@google.com> wrote:

On Tue, Jul 8, 2014 at 3:34 PM, Adam Klein <ad...@chromium.org> wrote:

IMHO, the behaviors are bad because it's not the correct way C++ language should work.

Can you expand on this argument (perhaps give an example where this operator is causing problems)? For all of the types where this has been done in Blink (to my knowledge), there is no other sense of equality than pointer equality. And so it seems perfectly within the realm of C++ to define operator== to test equality in an application-specific way.

I tend to agree that this kind of overload is confusing.

I think the Google style guide would suggest that, instead of using an overloaded operator==() here, using an explicitly-named method (e.g. ptrEquals()) would be clearer and safer.

[snip]

 

I said clearer, not cleaner.

Apologies, I blame my fuzzy eyes.

 

I definitely think the ptrEquals() call is clearer, functionally, than the use of == in your example above.  (isPtrEquivalent() would probably be an even better name.)

 

Can you supply an example where ptrEquals is safer?

It's safer in any case where a reader could imagine (accurately or not) objects being compared by value.

 

What does "compared by value" mean here? I've argued above that the definition of operator== for these types in Blink is semantically correct: it's asking if the two nodes are equal to each other.

How likely that is depends on a reader's familiarity with the code and types in question.  I tend to want to guarantee safety for people who are at a lower familiarity level, since they're the most likely to introduce bugs.

I'm having trouble coming up with an example where confusion over what operator== is doing would lead to bugs. I'll ask again: can someone provide one?

- Adam

[Peter Kasting]

OK, let me fall back on my less-friendly argument.

The Google style guide outright bans what you're doing ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Operator_Overloading ).  To the degree that Blink is slowly trying to reach some kind of style unity with Chromium over time, I think obeying the Google style guide is important.  So I think these operator overloads should be replaced.  I happen to also think that doing so makes these cases easier to understand, but I don't think that should actually matter.

PK

[Brett Wilson]

I agree with Peter.

Brett

[Jeffrey Yasskin]

Google style does tend to say that we should make up a name for
equality functions instead of using a consistent one. Doing this
introduces a speed bump in reading uses of classes, which helps people
who really want to pay attention to exactly what operations are being
invoked for each line of code, and hurts people who want to trust the
abstractions they're using. I tend to fall on the side who think that
we need to trust (and fix!) the abstractions in order to be able to
write large systems like Blink.

I appreciate seeing the authors of a type declare that the sensible
equality on instances is pointer equality, and I'd rather them say
that by defining operator== than by either making up a new name or by
commenting it. If they make up a new name, I have to wonder whether
there's some other name I'm supposed to use to check value equality,
and if they merely comment it, then I have to waste time checking the
documentation every time I see "&a==&b".

To be extra-clear: The Google style guide is outright harmful in cases
like this. It was written by C programmers and occasionally needs to
be ignored when we want to write maintainable C++ code.

All that said, defining "bool operator==(const T&, const T*)" is
sketchy. :) "it's not the correct way C++ language should work" is a
particularly bad argument against this, and I don't have any evidence
that it causes bugs or confusion, but saving a '*' character doesn't
really seem worth the unusual function, except as a transition path.

Jeffrey

[Adam Klein]

On Tue, Jul 8, 2014 at 4:03 PM, Peter Kasting <pkas...@google.com> wrote:

OK, let me fall back on my less-friendly argument.

Appeal to the style guide doesn't seem particularly unfriendly to me, and is certainly more direct.

 

The Google style guide outright bans what you're doing ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Operator_Overloading ).  To the degree that Blink is slowly trying to reach some kind of style unity with Chromium over time, I think obeying the Google style guide is important.  So I think these operator overloads should be replaced.

This Google style guide advice doesn't seem particularly well-followed:

/src/chromium/src$  git grep 'operator==' -- \*.h | wc -l

289

So I don't think "consistency with Chromium" works well here (I'll leave it to Jeffrey to fight out the merits of the style guide's recommendation here).

 

I happen to also think that doing so makes these cases easier to understand, but I don't think that should actually matter.

I'm interested, then, in what sort of evidence you'd admit for Blink's operator== overloads (or for Chromium's 289 operator== mentions).

- Adam

[Peter Kasting]

On Tue, Jul 8, 2014 at 4:05 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

If they make up a new name, I have to wonder whether

there's some other name I'm supposed to use to check value equality,

This seems like it contradicts your argument.  If a type is such that, in the reader's mind, it is a legitimate question whether value equality even makes sense, then having operator==() defined to do anything other than that seems inherently confusing and dangerous.

The only scenario where making operator==() do a pointer comparison makes sense is the scenario where value equality is completely semantically meaningless and invalid (which is precisely what's being argued here).  But in that case, readers shouldn't be "wondering if there's some other name they're supposed to use to check value equality".

 

To be extra-clear: The Google style guide is outright harmful in cases
like this. It was written by C programmers and occasionally needs to
be ignored when we want to write maintainable C++ code.

I would love to have the freedom to just make that argument about lots of things in the Google style guide I don't agree with.  However, I think we only get to make that call in exceptionally rare cases where the style guide is egregiously wrong and highly dangerous for some thing we have to be doing.  Otherwise, for better or worse, we've tried hard as a project (on the Chromium side) to obey the Google style guide.  I definitely do not agree with you that this is a case where violating the style guide makes Blink dramatically more maintainable.

PK 

[Elliott Sprehn]

On Tue, Jul 8, 2014 at 4:13 PM, 'Peter Kasting' via blink-dev <blin...@chromium.org> wrote:

On Tue, Jul 8, 2014 at 4:05 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

If they make up a new name, I have to wonder whether

there's some other name I'm supposed to use to check value equality,

[...]

 

To be extra-clear: The Google style guide is outright harmful in cases
like this. It was written by C programmers and occasionally needs to
be ignored when we want to write maintainable C++ code.

I would love to have the freedom to just make that argument about lots of things in the Google style guide I don't agree with.  However, I think we only get to make that call in exceptionally rare cases where the style guide is egregiously wrong and highly dangerous for some thing we have to be doing.  Otherwise, for better or worse, we've tried hard as a project (on the Chromium side) to obey the Google style guide.  I definitely do not agree with you that this is a case where violating the style guide makes Blink dramatically more maintainable.

We should not add ptrEquals, it just makes the code harder to read, our operator== is fine. In general Blink doesn't need to follow the Google style guide though, especially not in cases like this where it makes the code harder to read.

To answer the OP, the operator== is provided on all types where the concept of value equality and pointer equality are identical.

- E 

[Peter Kasting]

On Tue, Jul 8, 2014 at 4:12 PM, Adam Klein <ad...@chromium.org> wrote:

This Google style guide advice doesn't seem particularly well-followed:

/src/chromium/src$  git grep 'operator==' -- \*.h | wc -l

289

In a codebase of several million lines, 289 cases total is minimal -- especially when the style guide explicitly carves out an exception for classes that need to overload operator==() for the STL, which a large number of classes I've worked with in frontend code do.

Having fought many such style guide consistency battles before, I can assure you that this is a case where Chromium code does do a good job overall following the style recommendation.

PK

[Peter Kasting]

On Tue, Jul 8, 2014 at 4:25 PM, Elliott Sprehn <esp...@chromium.org> wrote:

In general Blink doesn't need to follow the Google style guide though

Certainly up to this point Blink has largely continued to chart its own stylistic path and not pay heed to other style guides.  However, many people at different times have proposed or recommended finding some sort of stylistic unification path between Blink and Chromium.  My suggestion supposes that this is, indeed, a desirable path.  Your statement is true if in fact Blink wants to continue being a completely isolated world style-wise.

PK 

[Adam Klein]

This seems like a (much) larger, and I doubt whether overloading operator== on a few classes in Blink is the most important divergence between Blink and Google style. Going down the style guide, I see some much bigger fish:

- Multiple inheritance

- Reference arguments

- Default arguments

If your real argument against operator== is that Blink should follow the Google style guide, that seems like a separate thread to me. I think Elliott, Jeffrey and myself have all provided reasons why Blink provides these operators.

- Adam

[Peter Kasting]

On Tue, Jul 8, 2014 at 4:48 PM, Adam Klein <ad...@chromium.org> wrote:

This seems like a (much) larger, and I doubt whether overloading operator== on a few classes in Blink is the most important divergence between Blink and Google style. Going down the style guide, I see some much bigger fish:

- Multiple inheritance

- Reference arguments

- Default arguments

If your real argument against operator== is that Blink should follow the Google style guide, that seems like a separate thread to me.

I don't think it's an all-or-nothing thing -- indeed, if it were, I think clearly it would never happen.

We've made a variety of changes since the Blink fork to bring individual style rules more into alignment with Google/Chromium, one at a time.  I am suggesting that people consider doing so for this case as well.  It would be a mistake to try and tie this to a large, synchronized effort to change lots of things at once, but it makes sense to recommend it if the topic is up for discussion anyway.

PK

[Xianzhu Wang]

Discussions until now:

- operator==(ref, ref): Still controversial. (One question: Does *node->cloneNode() == *node look like true or false to you intuitively?)

- operaotr==(ptr, ref) and operator==(ref, ptr): Most people who talked about this seem to think this is not the best. Do you agree to remove them first? (I could do it).

[Viet-Trung Luu]

I agree that adding ptrEquals is silly.

 

To answer the OP, the operator== is provided on all types where the concept of value equality and pointer equality are identical.

From a practical standpoint, I wonder how much these operator==s are actually used (especially the surprising reference+pointer variants), versus the unambiguous &foo == &bar (when foo, bar are references), &foo == bar (when foo is a reference, bar is a pointer), etc.

The benefit of &foo == &bar (etc.) to non-blinker ignoramuses like me is that it's clear at the comparison site what's being checked. The cost is obviously some extra &s. (Also, with the operator==s, you may lose consistency, which could add confusion, since some people may still do &foo == &bar.)

 

- E 

[Peter Kasting]

On Tue, Jul 8, 2014 at 6:08 PM, Viet-Trung Luu <viettr...@chromium.org> wrote:

On Tue, Jul 8, 2014 at 4:25 PM, Elliott Sprehn <esp...@chromium.org> wrote:

We should not add ptrEquals, it just makes the code harder to read,

I agree that adding ptrEquals is silly.

To answer the OP, the operator== is provided on all types where the concept of value equality and pointer equality are identical.

From a practical standpoint, I wonder how much these operator==s are actually used (especially the surprising reference+pointer variants), versus the unambiguous &foo == &bar (when foo, bar are references), &foo == bar (when foo is a reference, bar is a pointer), etc.

The benefit of &foo == &bar (etc.) to non-blinker ignoramuses like me is that it's clear at the comparison site what's being checked. The cost is obviously some extra &s.

I agree that just removing these and asking callers to do &foo == &bar (with &s omitted for pointers) is a better solution than either what's going on today or adding something like ptrEquals().  It's nearly as compact as using == directly, and it's unambiguous and can't possibly cause any unexpected side effects.

PK 

[Eric Seidel]

==(const&, const&) seems fine for NonCopyable types like RenderObject
or Node which are not expected to have value equivalence (and which
were always held by pointer until recently in the grand
make-everything-references style-sweep).

It's much more important to me that the references-rewrite reach a
stable state and we have less &foo and needless "if (foo && foo->bar)"
in our codebase.

I've not yet heard of someone being confused by this quirk. Until I
do, I'm going to continue not to worry. We have much bigger fish to
fry.

Yes, I would like our style to agree more with Chromium's overtime,
because I believe there is value in making it easy for someone to hack
on both codebases. That said, such style agreement pretty far down on
my list of concerns right now.

If someone feels passionate about this, I would encourage them to
worry first about getting our code in the same place, before worrying
about style divergences:
https://docs.google.com/a/chromium.org/document/d/1QDkMgQB1BuQixXHYNrcMRRJ--EdlaCKvie9hdD2qR90/edit

[Elliott Sprehn]

The & is just line noise, there's no other meaning when you see node == node, &node == node or &node == &node. Node's can only be compared one way (by address).

I don't think there's anything about this bike shed that needs changing. We have much bigger fish to fry.

- E

[Erik Corry]

The 'need' for the operator only arises because of the widespread use
of non-const refs in Blink, to indicate a non-nullable pointer. I
think this is a mistake for several reasons:

* It violates the Google style guide that does not allow non-const refs
- There are good reasons for this rule eg. that non-const refs are
confusing at the call site.
- Blink is getting more and more non-const refs, so it is moving
_away_ from the Google style guide
* It doesn't work with Oilpan transition types, because you can't
override '.', only '->'
* It leads to these == operators. Because people are thinking of refs
as non-nullable pointers they expect to be able to compare them with
pointers.
* It doesn't actually tell you that the pointer is not null any more
than using -> on a pointer does

The last point perhaps bears elaboration. Consider two versions of a function.

// Ref version.
foo(*b); // Undefined behaviour happens here if b is null.
void foo(Bar& b) // This tells us that b is not null, though actually
in practice it may be.
{
b.m(); // This is where the crash actually happens in practice.
}

// Pointer version.
foo(b);
void foo(Bar* b)
{
b->m(); // This tells us that b is not null, and this is where
undefined behaviour happens if b is null
}

In both cases, if b is actually null, then undefined behaviour is
encountered. In practice the compiler generates the same code in both
cases, and the crash happens in the same place, at the call to m().

There are more and more examples where we create a ref from a pointer
using the * operator because we know it's not null at that point.

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

--
Erik Corry
Google Denmark ApS
Sankt Petri Passage 5, 2. sal
1165 København K - Denmark - CVR nr. 28 86 69 84

[Nico Weber]

(Note that we have an ubsan bot that catches references to NULL at the point where they are formed, not where when they're used – https://code.google.com/p/chromium/issues/detail?id=174801 )

[Adam Barth]

On Wed Aug 20 2014 at 7:21:56 AM 'Erik Corry' via blink-dev <blin...@chromium.org> wrote:

The 'need' for the operator only arises because of the widespread use
of non-const refs in Blink, to indicate a non-nullable pointer. I
think this is a mistake for several reasons:

* It violates the Google style guide that does not allow non-const refs
   - There are good reasons for this rule eg. that non-const refs are
confusing at the call site.
   - Blink is getting more and more non-const refs, so it is moving
_away_ from the Google style guide

It's important to understand that Blink doesn't follow the Google style guide.

 

* It doesn't work with Oilpan transition types, because you can't
override '.', only '->'

That seems like a temporary problem that will resolve itself once the transition is over.

 

* It leads to these == operators. Because people are thinking of refs
as non-nullable pointers they expect to be able to compare them with
pointers.

That doesn't seem like much of a problem.

 

* It doesn't actually tell you that the pointer is not null any more
than using -> on a pointer does

Before we used non-const references, we had many redundant null checks of pointers that could not possibly be null.  Using references let us systematically remove these impossible-to-reach conditions and to prevent them from being re-introduced over time.

 

The last point perhaps bears elaboration.  Consider two versions of a function.

// Ref version.
foo(*b); // Undefined behaviour happens here if b is null.
void foo(Bar& b) // This tells us that b is not null, though actually
in practice it may be.
{
  b.m(); // This is where the crash actually happens in practice.
}

// Pointer version.
foo(b);
void foo(Bar* b)
{
  b->m(); // This tells us that b is not null, and this is where
undefined behaviour happens if b is null
}

In both cases, if b is actually null, then undefined behaviour is
encountered. In practice the compiler generates the same code in both
cases, and the crash happens in the same place, at the call to m().

There are more and more examples where we create a ref from a pointer
using the * operator because we know it's not null at that point.

A good case to consider is RenderObject::document.  That returns a Document&, which means we no longer have any null checks of the return value of this function and no one will introduce one.

Now consider RenderObject::frame.  That returns a LocalFrame*.  Should you null check the result of this function?  My guess is that the majority of the people contributing to this project don't know the answer to that question.

Adam

> email to blink-dev+unsubscribe@chromium.org.

[James Robinson]

On Wed, Aug 20, 2014 at 7:21 AM, 'Erik Corry' via blink-dev <blin...@chromium.org> wrote:

The 'need' for the operator only arises because of the widespread use
of non-const refs in Blink, to indicate a non-nullable pointer. I
think this is a mistake for several reasons:

* It violates the Google style guide that does not allow non-const refs

This is a widely-held misconception but it is completely wrong.  The google style guide bans only the use of non-const refs as function parameters.  It has no position on non-const refs as return types, member variables, locals, or anything else.  Blink (as WebKit before it) has used non-const refs for out parameters for the history of the project and that hasn't really changed very much lately.  The nullity argument for function parameters rarely comes up in practice - in cases where an optional out parameter is needed in Blink a pointer is used and it's pretty clear to everyone what that means.  The issue with non-const reference parameters being ambiguous at callsites is real but it isn't changing with these changes.  The style guide has no arguments against the changes being discussed here.

  

   - There are good reasons for this rule eg. that non-const refs are
confusing at the call site.
   - Blink is getting more and more non-const refs, so it is moving
_away_ from the Google style guide

No it isn't, and in any case there's no plan to move Blink to use the Google C++ style so it's really a non-sequitur anyway.

- James