base::Version "Equals" method vs operator==

[Rob Percival]

I'm curious why base::Version has an "Equals" method, rather than an overload for operator==. Is anyone aware of the design rationale? Similarly, the "IsOlderThan" method could have been implemented as operator<. I've just noticed their absence when writing tests involving base::Version and GTest complaining that it can't test them for equality. The style guide says "Don't go out of your way to avoid defining operator overloads. For example, prefer to define ==, =, and <<, rather than Equals(), CopyFrom(), and PrintTo()."

[Ryan Sleevi]

On Wed, Dec 16, 2015 at 9:54 AM, Rob Percival <robpe...@chromium.org> wrote:

I'm curious why base::Version has an "Equals" method, rather than an overload for operator==. Is anyone aware of the design rationale? Similarly, the "IsOlderThan" method could have been implemented as operator<. I've just noticed their absence when writing tests involving base::Version and GTest complaining that it can't test them for equality. The style guide says "Don't go out of your way to avoid defining operator overloads. For example, prefer to define ==, =, and <<, rather than Equals(), CopyFrom(), and PrintTo()."

The Style Guide used to say the opposite is why.

That change (to the current text) was only made in the past 3 months. For the past N years before, it advised what you see.

https://github.com/google/styleguide/commit/3591b2e540cbcb07423e02d20eee482165776603 for the relevant change.

[Thiago Farina]

On Wednesday, December 16, 2015, Rob Percival <robpe...@chromium.org> wrote:

I'm curious why base::Version has an "Equals" method, rather than an overload for operator==. Is anyone aware of the design rationale? 

I think the style guide [1] has changed, I remember it suggesting to use Equals rather than overloading operator== because that is easier to grep. But now it says the opposite, to use operator== rather than Equals.

 

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

--

Thiago Farina

[Rob Percival]

Ah ok. Would it be a welcome change to add an operator== overload to base::Version? Also, should I follow the style guide and implement operator== for new classes (where appropriate), or prioritise consistency and implement Equals methods?

[Brett Wilson]

On Wed, Dec 16, 2015 at 10:34 AM, 'Rob Percival' via Chromium-dev <chromi...@chromium.org> wrote:

Ah ok. Would it be a welcome change to add an operator== overload to base::Version? Also, should I follow the style guide and implement operator== for new classes (where appropriate), or prioritise consistency and implement Equals methods?

I don't think there's much consistency about how the equals functions work so I would just follow the style guide for new code. If this comes up often you're probably doing it wrong anyway :)

I would personally be OK with changing Version but would want to be sure that we removed the old versions so there aren't two ways of doing the same thing. Also, I don't think we should be doing a lot of updating. In addition to being a waste of time some things can get confusing. An example is base::Value where it looks obvious, but operator== is likely to be error-prone because Values are usually passed by pointer and it will be easy to do pointer comparisons by accident.

Brett

[Thiago Farina]

On Wednesday, December 16, 2015, Rob Percival <robpe...@google.com> wrote:

Ah ok. Would it be a welcome change to add an operator== overload to base::Version? 

That helps the gtest case right? Are you sure there aren't existing tests doing the same you are trying to do without the operator==?

Are you proposing to just add operator== along side the existing Equals method or to replace Equals by the operator?

--

Thiago Farina

[Rob Percival]

I'm just trying to get a feeling for what the preferred approach is in this sort of situation (I'm new to Chromium dev). Replacing the Equals method is cleaner, but obviously a far bigger change, whereas just adding the operator would allow a more gradual transition. The latter does result in there being two ways of testing equality though, as Brett points out and wishes to avoid.

Personally, I'm tempted to just leave it as it is. I can still test equality of Versions, just not with EXPECT_EQ (so the failure message won't be as useful, but that's not a big deal). I was most interested in whether there'd be strong opinions one way or another. Am I right in concluding that updating code simply for the sake of making it compliant with updates to the style guide is discouraged, but new code should be written as such?

Thanks for your input everyone.

[Peter Kasting]

On Wed, Dec 16, 2015 at 2:27 PM, 'Rob Percival' via Chromium-dev <chromi...@chromium.org> wrote:

I'm just trying to get a feeling for what the preferred approach is in this sort of situation (I'm new to Chromium dev). Replacing the Equals method is cleaner, but obviously a far bigger change, whereas just adding the operator would allow a more gradual transition. The latter does result in there being two ways of testing equality though, as Brett points out and wishes to avoid.

Personally, I'm tempted to just leave it as it is. I can still test equality of Versions, just not with EXPECT_EQ (so the failure message won't be as useful, but that's not a big deal). I was most interested in whether there'd be strong opinions one way or another. Am I right in concluding that updating code simply for the sake of making it compliant with updates to the style guide is discouraged, but new code should be written as such?

New code should almost always comply with the current style guide (we try to call out the rare exceptions, e.g. enum value names, in the Chromium style guide), and that definitely applies here.

As for updating existing code, it's always a judgment call.  The main downsides to it are engineer and reviewer time, and the risk of introducing bugs.  In this case, besides consistency, there is at least one other upside: the ability to use EXPECT_EQ.

Often, the conclusion we come to is "don't go about updating code just to update it, but if you're touching that code or nearby/related code anyway, it's OK to go ahead and clean things up while you're there".  In a case like this, I would say it's up to you whether you really want to update; if you are "tempted to leave it as is" I don't think there's anything wrong with that.

PK

[Scott Hess]

I'd make the change if it were compelling.  For instance, if there were a bunch of cases where something like this was being passed to std::sort() or std::equal_range(), having operator< is cleaner than passing a comparator.

In this case, I expect if you implemented operator==(), then you'd find EXPECT_EQ() also wants operator<<() to log the values.  Unless I mis-remember.

-scott

On Wed, Dec 16, 2015 at 2:27 PM, 'Rob Percival' via Chromium-dev <chromi...@chromium.org> wrote:

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Brett Wilson]

On Wed, Dec 16, 2015 at 2:27 PM, 'Rob Percival' via Chromium-dev <chromi...@chromium.org> wrote:

I'm just trying to get a feeling for what the preferred approach is in this sort of situation (I'm new to Chromium dev). Replacing the Equals method is cleaner, but obviously a far bigger change, whereas just adding the operator would allow a more gradual transition. The latter does result in there being two ways of testing equality though, as Brett points out and wishes to avoid.

People like me often push for the extra work to change everything at once since in reality nobody ever goes back and finishes these "gradual transitions." base::Version is not used in so many places that updating it all at once is unreasonable. And if you're not willing to put in that time, then you're definitely not going to come back later and update the rest of the callers.

 

Personally, I'm tempted to just leave it as it is. I can still test equality of Versions, just not with EXPECT_EQ (so the failure message won't be as useful, but that's not a big deal). I was most interested in whether there'd be strong opinions one way or another. Am I right in concluding that updating code simply for the sake of making it compliant with updates to the style guide is discouraged, but new code should be written as such?

You should feel free to leave it if you don't want to update it.

Brett

[Rob Percival]

FYI, I've gotten around to making this change: https://codereview.chromium.org/1575523002/

[Dmitry Skiba]

I wonder if it's possible to create global template operator== which only enables itself if type has Equals method.

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Rob Percival]

With enable_if, that's certainly possible, though I'm not sure ADL would always find it. Brett wasn't in favour of having two ways of doing the same thing though.

[Brett Wilson]

On Fri, Jan 8, 2016 at 11:01 AM, 'Dmitry Skiba' via Chromium-dev <chromi...@chromium.org> wrote:

I wonder if it's possible to create global template operator== which only enables itself if type has Equals method.

That sounds like the type of obscure magic that is best avoided.

Brett