Comparable: Enable comparison operators based on a ternary comparison

[SolidWallOfCode]

Given a ternary comparison (e.g., -1, 0, 1 result), this creates the 6 standard binary comparison operators. This works for same type and cross type comparisons.

Depends on #4140. An alternative approach for #4262.

[ywkaras]

There is a cheesy tick you have to use if both a base class and a class derived from it both are comparable, illustrated in this code;

#if 1
class Tricky : public ts::Comparable {};
#else
using Tricky = ts::Comparable;
#endif

class X : public ts::Comparable {};

int cmp(const X &, int) { return 0; }

class Y : public X , public Tricky {};

int cmp(const Y &, int) { return 0; }

bool foo(Y &b1) { return b1 == 5; }

If Y inherits directly from ts::comparable, you get this warning from gcc (that we treat as an error):

unit_tests/test_ts_meta.cc:260:7: error: direct base 'ts::Comparable' inaccessible in 'Y' due to ambiguity [-Werror]

[SolidWallOfCode]

I'll take a look at that.

[SolidWallOfCode]

Two things -

For Y, why inherit from ts::Comparable? If the base class is Comparable, that suffices. In other words, "Doctor, it hurts when I do this".

This can also be fixed by using virtual base classes. I suppose the recommendation could be to always use public virtual ts::Comparable which shouldn't cause any problems and would avoid this issue.

Because of these two (IMHO easy to implement) avoidance strategies, I dont' think this rates as a real problem.

P.S. I added some test cases to demonstrate these points. I verified that if the virtual is removed from the Foxtrot and Golf classes, it doesn't compile due to the warning you noted.

[ywkaras]

A virtual base class adds a hidden v pointer to every class instance if it's not already present.

[ywkaras]

Consider using std::is_convertable in place of std::is_base_of . This allows the alternative of tagging a class for comparison using a dummy conversion operator:

operator Comparable ();

This avoids the ambiguous base class error if both a base class and a class derived from it need to be tagged as comparable. It also allows a derived class to suppress inheritance of the comparison operators of a base class with:

operator Comparable () = delete;

https://godbolt.org/z/ToweoI

[SolidWallOfCode]

I considered that but think it's a bad idea because it could easily cause the Comparable machinery to fire up in places it shouldn't. "if both a base class and a class derived from it need to be tagged as comparable" - IMHO that is never the case. If the base class is tagged as comparable, there is never a need to tag a class derived from it. Just don't do it. That seems a simpler solution.

[zwoop]

This is kinda crazy, I really don't think we should go into this land of obscurity.

[SolidWallOfCode]

I think it seems quite non-obscure and useful. Why would you favor having to write out the standard comparison operators by hand, when they're all completely boiler plate? It's an easy place to make mistakes and we have in fact had bugs for that reason. This is exactly the kind of mindless programming that should be automated.

[ywkaras]

Why aren't the unit tests for this in src/tscpp/util/unit_tests?

[ywkaras]

Obscurity #1: The test_ts_meta.cc still compiles if I make this change to it:

wkaras ~/REPOS/TS
$ git diff
diff --git a/src/tscore/unit_tests/test_ts_meta.cc b/src/tscore/unit_tests/test_ts_meta.cc
index 114f74a62..edae528aa 100644
--- a/src/tscore/unit_tests/test_ts_meta.cc
+++ b/src/tscore/unit_tests/test_ts_meta.cc
@@ -119,7 +119,7 @@ cmp(Alpha const &lhs, Alpha const &rhs)
 }
 
 int
-cmp(Alpha const &lhs, int rhs)
+comparable(Alpha const &lhs, int rhs)
 {
   return lhs._n - rhs;
 }

[ywkaras]

Obscurity #2: If I change test_ts_meta.cc like this:

wkaras ~/REPOS/TS
$ git diff
diff --git a/src/tscore/unit_tests/test_ts_meta.cc b/src/tscore/unit_tests/test_ts_meta.cc
index 114f74a62..90eaadb84 100644
--- a/src/tscore/unit_tests/test_ts_meta.cc
+++ b/src/tscore/unit_tests/test_ts_meta.cc
@@ -113,7 +113,7 @@ struct Alpha : public ts::Comparable {
 };
 
 int
-cmp(Alpha const &lhs, Alpha const &rhs)
+compare(Alpha const &lhs, Alpha const &rhs)
 {
   return lhs._n - rhs._n;
 }

(An error that one could easily make in a moment of absent-mindedness) the resulting compiler error messages are:

C$ make check
make  test_atomic test_freelist test_geometry test_X509HostnameValidator test_tscore
make[1]: Entering directory '/Users/wkaras/REPOS/TS/src/tscore'
make[1]: 'test_atomic' is up to date.
make[1]: 'test_freelist' is up to date.
make[1]: 'test_geometry' is up to date.
make[1]: 'test_X509HostnameValidator' is up to date.
  CXX      unit_tests/test_tscore-test_ts_meta.o
In file included from unit_tests/test_ts_meta.cc:24:
../../include/tscpp/util/Comparable.h: In instantiation of 'typename std::enable_if<(std::is_base_of<ts::Comparable, T>::value || std::is_base_of<ts::Comparable, U>::value), bool>::type ts::operator==(const T&, const U&) [with T = Alpha; U = Alpha; typename std::enable_if<(std::is_base_of<ts::Comparable, T>::value || std::is_base_of<ts::Comparable, U>::value), bool>::type = bool]':
unit_tests/test_ts_meta.cc:241:3:   required from here
../../include/tscpp/util/Comparable.h:153:41: error: no matching function for call to 'ComparableFunction(const Alpha&, const Alpha&, const ts::meta::CaseTag<9>&)'
   return 0 == detail::ComparableFunction(lhs, rhs, meta::CaseArg);
               ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from unit_tests/test_ts_meta.cc:24:
../../include/tscpp/util/Comparable.h:51:3: note: candidate: 'template<class T, class U> decltype ((ts::ComparablePolicy<T, U>()(lhs, rhs), int())) ts::detail::ComparableFunction(const T&, const U&, const ts::meta::CaseTag<4>&)'
   ComparableFunction(T const &lhs, U const &rhs, meta::CaseTag<4> const &) -> decltype(ComparablePolicy<T, U>()(lhs, rhs), int())
   ^~~~~~~~~~~~~~~~~~
../../include/tscpp/util/Comparable.h:51:3: note:   template argument deduction/substitution failed:
../../include/tscpp/util/Comparable.h: In substitution of 'template<class T, class U> decltype ((ts::ComparablePolicy<T, U>()(lhs, rhs), int())) ts::detail::ComparableFunction(const T&, const U&, const ts::meta::CaseTag<4>&) [with T = Alpha; U = Alpha]':
../../include/tscpp/util/Comparable.h:153:41:   required from 'typename std::enable_if<(std::is_base_of<ts::Comparable, T>::value || std::is_base_of<ts::Comparable, U>::value), bool>::type ts::operator==(const T&, const U&) [with T = Alpha; U = Alpha; typename std::enable_if<(std::is_base_of<ts::Comparable, T>::value || std::is_base_of<ts::Comparable, U>::value), bool>::type = bool]'
unit_tests/test_ts_meta.cc:241:3:   required from here
../../include/tscpp/util/Comparable.h:51:112: error: no match for call to '(ts::ComparablePolicy<Alpha, Alpha>) (const Alpha&, const Alpha&)'
   ComparableFunction(T const &lhs, U const &rhs, meta::CaseTag<4> const &) -> decltype(ComparablePolicy<T, U>()(lhs, rhs), int())
                                                                                        ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
In file included from unit_tests/test_ts_meta.cc:24:
../../include/tscpp/util/Comparable.h: In instantiation of 'typename std::enable_if<(std::is_base_of<ts::Comparable, T>::value || std::is_base_of<ts::Comparable, U>::value), bool>::type ts::operator==(const T&, const U&) [with T = Alpha; U = Alpha; typename std::enable_if<(std::is_base_of<ts::Comparable, T>::value || std::is_base_of<ts::Comparable, U>::value), bool>::type = bool]':
unit_tests/test_ts_meta.cc:241:3:   required from here
../../include/tscpp/util/Comparable.h:59:3: note: candidate: 'template<class T, class U> decltype ((cmp(lhs, rhs), int())) ts::detail::ComparableFunction(const T&, const U&, const ts::meta::CaseTag<3>&)'
   ComparableFunction(T const &lhs, U const &rhs, meta::CaseTag<3> const &) -> decltype(cmp(lhs, rhs), int())
   ^~~~~~~~~~~~~~~~~~
../../include/tscpp/util/Comparable.h:59:3: note:   template argument deduction/substitution failed:
../../include/tscpp/util/Comparable.h: In substitution of 'template<class T, class U> decltype ((cmp(lhs, rhs), int())) ts::detail::ComparableFunction(const T&, const U&, const ts::meta::CaseTag<3>&) [with T = Alpha; U = Alpha]':
../../include/tscpp/util/Comparable.h:153:41:   required from 'typename std::enable_if<(std::is_base_of<ts::Comparable, T>::value || std::is_base_of<ts::Comparable, U>::value), bool>::type ts::operator==(const T&, const U&) [with T = Alpha; U = Alpha; typename std::enable_if<(std::is_base_of<ts::Comparable, T>::value || std::is_base_of<ts::Comparable, U>::value), bool>::type = bool]'
unit_tests/test_ts_meta.cc:241:3:   required from here
../../include/tscpp/util/Comparable.h:59:91: error: no matching function for call to 'cmp(const Alpha&, const Alpha&)'
   ComparableFunction(T const &lhs, U const &rhs, meta::CaseTag<3> const &) -> decltype(cmp(lhs, rhs), int())
                                                                                        ~~~^~~~~~~~~~
unit_tests/test_ts_meta.cc:122:1: note: candidate: 'int cmp(const Alpha&, int)'
 cmp(Alpha const &lhs, int rhs)
 ^~~
...

(Additional 420 lines of diagnostic output deleted for brevity.)

[SolidWallOfCode]

1. Because I was originally using this to test ts::meta. I should probably move the tests.

2. It's likely that compiles because that comparison isn't used.

3. That's just the way it is. I would like to work on a better error message but I'm not sure it's possible. One option I've considered is to shift the cases up and add a new default case that fails in a better way. OTOH would have some side effects I'm not sure I like (such as preventing any other similar feature from working). At this point I think I'll just document what the first error is, because that's what matters. E.g.

"If you see this"

error: no matching function for call to 'ComparableFunction(const Alpha&, const Alpha&, const ts::meta::CaseTag<9>&)'
   return 0 == detail::ComparableFunction(lhs, rhs, meta::CaseArg);

"then you have not provided the correct ternary compare operator."

[SolidWallOfCode]

I had a chance to experiment on my theory about why the change

-cmp(Alpha const &lhs, int rhs)
+comparable(Alpha const &lhs, int rhs)

still compiles. I was half right. It continues to work for 2 different reasons.

1. Because the Alpha constructor is not explicit, the int argument can be promoted to an Alpha. This is what I thought was going on.

2. The method search logic will check for a cmp with flipped arguments, so even after this change there is still a cmp(int lhs, Alpha const& rhs) function which is found and used. I noticed this after I added the explicit and it still worked.

Changing both functions and adding explicit does finally fail to compile.

[SolidWallOfCode]

I've redone the PR a bit.

• The meta support was moved over to "tscpp/util" because it will be needed in plugins, particularly CPP API plugins, because the latter will need Comparable which depends on this.
• I split the Comparable tests out separately and moved those tests, and the meta tests, over to "tscpp/util/unit_tests".
• I tweaked the tests to be clearer about the points raised by @ywkaras.

[ywkaras]

Good changes, but this does support @zwoop 's point about obscurity. What do you see as the worse problems with the approach using macros, that outweigh these problems?

[SolidWallOfCode]

I don't see any of this as a big problem. I think the excessive error messages are better than obscure and hard to detect failures, especially with a bit of documentation. Note such errors are basically one offs - they would happen very early in the use cycle and once fixed don't recur. The other changes are basically just testing, not actual interface changes. Other advantages over macros are

• Clean namespace support. This is a big deal to me, that the functionality can be constrained to interact nicely with other C++ mechanisms.
• Bettter support for function argument conversions.
• It's more robust and flexible in terms of how the ternary compare is done.
• It's easier to use for the client - one inherit, one compare implementation, and it's done.

[ywkaras]

I don't see the point about the argument conversions. The ease of use comes at a cost. If you make types A and B comparable, and type C comparable with itself, you get the scary error spew if you compare A with C, B with C, A with itself. It only takes one line of code to make a free-standing cmp() that calls a member cmp().

I think the scenarios where lack of macro scoping could cause run-time errors are rare. I think they'd mostly result from defining an parameterless macro to translate into a single identifier. The compile-time errors I doubt would be harder to deal with than what's above.

I think C++ is unavoidably a hodgepodge. Templates should be preferred over macros, but not fetishized. There are reasons why the preprocessor has not been fully deprecated.

[SolidWallOfCode]

Lack of macro scoping and runtime errors introduced by bad use of macros are independent issues, one is not cause by the other. In your own example, the key point is only 3 lines in to the error list - "unit_tests/test_ts_meta.cc:241:3: required from here".

I would also note that ascribing opinions that differ from yours to psychological problems like "fetishized" is counter productive, because that is the refugee of people who don't have substantive arguments.

[ywkaras]

Sorry I didn't mean it literally.

[SolidWallOfCode]

I find macros very problematic and (for me at least) noticeable worse than all but the most complex template implementation. Generally the template issue is quite early in the error text and so most of it can be ignored. I updated the comments to make the error output a bit clearer.

I also find template work to be more flexible and adaptable and I place a high value on those attributes.

[ywkaras]

Something else that gives me heartburn is trying to understand the C++ rules for using names that come from the surrounding context when using templates (like "cmp" in this PR). Here is an example:

wkaras ~/REPOS/OS_MISC/TEMPLATE_CONTEXT
$ cat x.cc
#include <iostream>

template <typename T>
void tpl_func(T x) { foo(x); }

#if PRIMITIVE

using A = int;

#else

struct A {};

#endif

void foo(A) { std::cout << "i x\n"; }

void bar()
{
  tpl_func(A());
}
wkaras ~/REPOS/OS_MISC/TEMPLATE_CONTEXT
$ cat bld.sh
gcc -pedantic -Wall -Wextra -std=c++17 -DPRIMITIVE=$PRIMITIVE -c x.cc
wkaras ~/REPOS/OS_MISC/TEMPLATE_CONTEXT
$ PRIMITIVE=0 ./bld.sh
wkaras ~/REPOS/OS_MISC/TEMPLATE_CONTEXT
$ PRIMITIVE=1 ./bld.sh
x.cc:4:22: error: call to function 'foo' that is neither visible in the template definition nor found by argument-dependent lookup
void tpl_func(T x) { foo(x); }
                     ^
x.cc:20:3: note: in instantiation of function template specialization 'tpl_func<int>' requested here
  tpl_func(A());
  ^
x.cc:16:6: note: 'foo' should be declared prior to the call site
void foo(A) { std::cout << "i x\n"; }
     ^
1 error generated.
wkaras ~/REPOS/OS_MISC/TEMPLATE_CONTEXT
$ 

[ywkaras]

Does this mean you're bending the knee to #4262 ?