Provide comparison operators for map/set/etc. instead of _Tree

[StephanTLavavej]

Fixes #215. I am not actually sure whether this is observable in C++20, but doing what the Standard depicts is generally a good idea, and will make it easier to develop and validate the spaceship operator changes.

Test coverage is provided by std:

STL/tests/std/include/instantiate_containers_iterators_common.hpp

Lines 70 to 83 in 550713e

	template <typename T, typename U>
	void comparable_test(T&& lhs, U&& rhs) {
	equality_test(lhs, rhs);
	USE_VALUE(lhs < rhs);
	USE_VALUE(lhs > rhs);
	USE_VALUE(lhs <= rhs);
	USE_VALUE(lhs >= rhs);
	}
	template <typename T>
	void comparable_test(T value) {
	T another = construct_another(value);
	comparable_test(value, another);
	}

STL/tests/std/tests/VSO_0000000_instantiate_containers/test.cpp

Lines 442 to 446 in 550713e

	void map_test() {
	map<int, int> value{};
	tree_baseclass_test(value);
	swap_test(value);
	comparable_test(value);

and the tr1 legacy test suite:

STL/tests/tr1/tests/map/test.cpp

Lines 195 to 200 in 550713e

	CHECK(v1 == v1);
	CHECK(v0 < v1);
	CHECK(v0 != v1);
	CHECK(v1 > v0);
	CHECK(v0 <= v1);
	CHECK(v1 >= v0);

(Curiously, I was unable to find where libcxx tests the container comparisons, but I didn't search very intensively.)

[CaseyCarter]

(Curiously, I was unable to find where libcxx tests the container comparisons, but I didn't search very intensively.)

This came up on libcxx-dev in the last couple of weeks because someone was working on spaceship: they don't, and no one noticed before.

[MikeGitb]

Does the standard wording allow them to be defined as hidden friends instead? My understanding is that this would lead to less compiler spew if it can't find an appropriate overload for comparison operators and a standard container is involved.

[StephanTLavavej]

While hidden friends would both improve compiler throughput and diagnostic quality, they would also break existing code in certain cases. Example:

C:\Temp>type meow.cpp
namespace Std {
#ifdef HIDDEN_FRIEND
    template <typename T>
    struct Vector {
        friend bool operator==(const Vector&, const Vector&) {
            return true;
        }
    };
#else // HIDDEN_FRIEND
    template <typename T>
    struct Vector {};
    template <typename T>
    bool operator==(const Vector<T>&, const Vector<T>&) {
        return true;
    }
#endif // HIDDEN_FRIEND
} // namespace Std

struct User {
    User() = default;
    User(const Std::Vector<int>&) {}
    operator Std::Vector<int>() const {
        return {};
    }
};

bool operator==(const User&, const User&) {
    return true;
}

int main() {
    User u;
    Std::Vector<int> v;
    (void) (u == u);
    (void) (v == v);
#ifndef SKIP_U_EQUALS_V
    (void) (u == v);
#endif
}

C:\Temp>cl /EHsc /nologo /W4 meow.cpp
meow.cpp

C:\Temp>cl /EHsc /nologo /W4 /DHIDDEN_FRIEND /DSKIP_U_EQUALS_V meow.cpp
meow.cpp

C:\Temp>cl /EHsc /nologo /W4 /DHIDDEN_FRIEND meow.cpp
meow.cpp
meow.cpp(37): error C2666: 'operator ==': 2 overloads have similar conversions
meow.cpp(27): note: could be 'bool operator ==(const User &,const User &)'
meow.cpp(5): note: or       'bool Std::Vector<int>::operator ==(const Std::Vector<int> &,const Std::Vector<int> &)' [found using argument-dependent lookup]
meow.cpp(37): note: while trying to match the argument list '(User, Std::Vector<int>)'

(Inspired by an example reduced from LLVM by Barry Revzin on March 8, 2019.)

[MikeGitb]

Yes, I'm aware that the two approaches aren't functionally equivalent. I was just wondering if the standard allowed both (explicitly or via some loophole) or explicitly required a regular free function version. But I could have looked up that myself. Sorry for the noise.

[StephanTLavavej]

It was an excellent question, not noise at all! I would have guessed that the difference wasn't observable, if I hadn't seen the counterexample. (It is also sometimes very hard to find the Standardese that forbids users from doing things like taking the address of Standard Library member functions, etc.)

[mnatsuhara]

Yay @ahanamuk is soooo happy about these changes 😄

[BillyONeal]

What about _Hash?

[StephanTLavavej]

@BillyONeal Excellent question. Fortunately, _Hash was unaffected by this issue - it doesn't attempt to provide comparison operators for its derived classes. (I vaguely recall that it might have in the past, but then we noticed that operator< was being provided for unordered_meow, and cleaned that up, and even purged it from hash_meow - but I might be misremembering.)

Here are the properly defined operators for unordered_meow:

STL/stl/inc/unordered_map

Lines 465 to 475 in 550713e

	template <class _Kty, class _Ty, class _Hasher, class _Keyeq, class _Alloc>
	_NODISCARD bool operator==(const unordered_map<_Kty, _Ty, _Hasher, _Keyeq, _Alloc>& _Left,
	const unordered_map<_Kty, _Ty, _Hasher, _Keyeq, _Alloc>& _Right) {
	return _Hash_equal(_Left, _Right);
	}
	template <class _Kty, class _Ty, class _Hasher, class _Keyeq, class _Alloc>
	_NODISCARD bool operator!=(const unordered_map<_Kty, _Ty, _Hasher, _Keyeq, _Alloc>& _Left,
	const unordered_map<_Kty, _Ty, _Hasher, _Keyeq, _Alloc>& _Right) {
	return !(_Left == _Right);
	}

STL/stl/inc/unordered_map

Lines 755 to 765 in 550713e

	template <class _Kty, class _Ty, class _Hasher, class _Keyeq, class _Alloc>
	_NODISCARD bool operator==(const unordered_multimap<_Kty, _Ty, _Hasher, _Keyeq, _Alloc>& _Left,
	const unordered_multimap<_Kty, _Ty, _Hasher, _Keyeq, _Alloc>& _Right) {
	return _Hash_equal(_Left, _Right);
	}
	template <class _Kty, class _Ty, class _Hasher, class _Keyeq, class _Alloc>
	_NODISCARD bool operator!=(const unordered_multimap<_Kty, _Ty, _Hasher, _Keyeq, _Alloc>& _Left,
	const unordered_multimap<_Kty, _Ty, _Hasher, _Keyeq, _Alloc>& _Right) {
	return !(_Left == _Right);
	}

STL/stl/inc/unordered_set

Lines 319 to 329 in 550713e

	template <class _Kty, class _Hasher, class _Keyeq, class _Alloc>
	_NODISCARD bool operator==(const unordered_set<_Kty, _Hasher, _Keyeq, _Alloc>& _Left,
	const unordered_set<_Kty, _Hasher, _Keyeq, _Alloc>& _Right) {
	return _Hash_equal(_Left, _Right);
	}
	template <class _Kty, class _Hasher, class _Keyeq, class _Alloc>
	_NODISCARD bool operator!=(const unordered_set<_Kty, _Hasher, _Keyeq, _Alloc>& _Left,
	const unordered_set<_Kty, _Hasher, _Keyeq, _Alloc>& _Right) {
	return !(_Left == _Right);
	}

STL/stl/inc/unordered_set

Lines 581 to 591 in 550713e

	template <class _Kty, class _Hasher, class _Keyeq, class _Alloc>
	_NODISCARD bool operator==(const unordered_multiset<_Kty, _Hasher, _Keyeq, _Alloc>& _Left,
	const unordered_multiset<_Kty, _Hasher, _Keyeq, _Alloc>& _Right) {
	return _Hash_equal(_Left, _Right);
	}
	template <class _Kty, class _Hasher, class _Keyeq, class _Alloc>
	_NODISCARD bool operator!=(const unordered_multiset<_Kty, _Hasher, _Keyeq, _Alloc>& _Left,
	const unordered_multiset<_Kty, _Hasher, _Keyeq, _Alloc>& _Right) {
	return !(_Left == _Right);
	}

[BillyONeal]

Hmmm you might have asked me to add those before and that's the vague memory this ticked. Thanks!