what mystery? can you give more details?

In D8953#171062, @mwolff wrote:

what mystery? can you give more details?

There are two operator==() methods, one non-member and one non-const member one, which check different properties:

inline bool operator==( const KDevelop::VcsLocation& lhs, const KDevelop::VcsLocation& rhs )
{
    return( lhs.type() == rhs.type()
            && lhs.repositoryServer() == rhs.repositoryServer()
            && lhs.localUrl() == rhs.localUrl() );
}

and

bool VcsLocation::operator==( const KDevelop::VcsLocation& rhs )
{
    return( type() == rhs.type()
            && repositoryServer() == rhs.repositoryServer()
            && localUrl() == rhs.localUrl()
            && repositoryPath() == rhs.repositoryPath()
            && repositoryModule() == rhs.repositoryModule()
            && repositoryBranch() == rhs.repositoryBranch()
            && repositoryTag() == rhs.repositoryTag()
            && userData() == rhs.userData() );
}

They exist since their initial commit in 2007, but only the latter has been adapted on new property additions.

I cannot see why both exist and why with different logic.

It is indeed a mystery. Note that the member function isn't marked const, which it probably should be. Adding the const specifier produces a compiler error (about ambiguous function calls), as it should.

I think that either the method or the standalone function should go away, but which one? Right now the standalone function is used for the QHash<VcsLocation,QByteArray> members in VcsDiffPrivate, the method doesn't seem to be used at all. (Removing the implementation doesn't produce linker errors for me.) However, the implementation of the member operator== looks "more correct."

Remove the free function, make the member const and const&.

but in a separate patch please

So, any comment about the changes which are in this very patch? ;)

Or is it good to go, given it's following the patterm of the changes done before to the other VCS data container classes?

While OT for the content in this patch, yet a small reply:

In D8953#171486, @aaronpuchert wrote:

I think that either the method or the standalone function should go away, but which one? Right now the standalone function is used for the QHash<VcsLocation,QByteArray> members in VcsDiffPrivate, the method doesn't seem to be used at all. (Removing the implementation doesn't produce linker errors for me.) However, the implementation of the member operator== looks "more correct."

The non-member one might have been done exactly for the usage with the hash in VcsDiff (the qHash method for VcsLocation also only cares for the same subset of properties). I suspect the usage of the class for the hashmap might be a small abuse though, it could need another separate class designed for that very purpose, not sure yet. Then, my recent separate clean-up patch R32:36fd6bf0f877eaf491cdfbafcd0a9e86e743f764 has removed those hashmaps, so the usage scope of VcsLocation has been reduced and enables a more clear definition of its API by the remaining usage (already waiting in my local patch sets).

So I translate no comment on the content to no objections, and will push tomorrow (being confident about the change after all, as said it's consistent with the referenced one) :)