2
\$\begingroup\$

I'm trying to implement equality for all types that share same base.

Consider std::vector<unique_ptr<Shape>> and that we want to check if a certain given Shape& is equal to a certain shape managed in the vector. The idea is to have a generic solution through a base class, without relying on the exact content of the derived classes and without narrowing it to shapes (which is merely just a usage example).

I came to the following solution, with CRTP and would appreciate comments.

Code

AbstractBase

class AbstractBase {
    virtual bool base_equals(const AbstractBase& other) const = 0;
    virtual std::ostream& print(std::ostream& out = std::cout) const = 0;
public:
    bool operator==(const AbstractBase& other)const {
        if(typeid(*this)==typeid(other)) {
            return this->base_equals(other);
        }
        return false;
    }
    friend std::ostream& operator<<(std::ostream& out, const AbstractBase& ab) {
        out << "[" << (void*)&ab << "] ";
        return ab.print(out);
    }
};

CRTP Base

template<class ActualType>
class Base: public AbstractBase {
    virtual bool base_equals(const AbstractBase& other)const override final {
        return static_cast<const ActualType&>(*this)
                    == static_cast<const ActualType&>(other);
    }
};

Actual Classes

class A: public Base<A> {
    int num;
    virtual std::ostream& print(std::ostream& out = std::cout) const {
        return out << num;
    }
public:
    A(int i = 42): num(i) {}
    bool operator==(const A& a)const {
        return num == a.num;
    }
};

class B: public Base<B> {
    std::string str;
    virtual std::ostream& print(std::ostream& out = std::cout) const {
        return out << str;
    }
public:
    B(const std::string& s = "hello"): str(s) {}
    bool operator==(const B& b)const {
        return str == b.str;
    }
};

Simple Usage Example (Test)

int main() {
    A a1, a2, a3 = 5;
    B b1, b2, b3 = {"bye"};
    std::vector<const AbstractBase*> arr = {&a1, &a2, &a3, &b1, &b2, &b3};
    for(auto v1: arr) {
        for(auto v2: arr) {
            std::cout << *v1 << " == " << *v2 << " ? "
                      << std::boolalpha << (*v1 == *v2) << std::endl;
        }
    }
}

Output

[0x7ffefdb7f0b0] 42     ==  [0x7ffefdb7f0b0] 42     ?   true
[0x7ffefdb7f0b0] 42     ==  [0x7ffefdb7f0c0] 42     ?   true
[0x7ffefdb7f0b0] 42     ==  [0x7ffefdb7f0d0] 5      ?   false
[0x7ffefdb7f0b0] 42     ==  [0x7ffefdb7f0e0] hello  ?   false
[0x7ffefdb7f0b0] 42     ==  [0x7ffefdb7f110] hello  ?   false
[0x7ffefdb7f0b0] 42     ==  [0x7ffefdb7f140] bye    ?   false
[0x7ffefdb7f0c0] 42     ==  [0x7ffefdb7f0b0] 42     ?   true
[0x7ffefdb7f0c0] 42     ==  [0x7ffefdb7f0c0] 42     ?   true
[0x7ffefdb7f0c0] 42     ==  [0x7ffefdb7f0d0] 5      ?   false
[0x7ffefdb7f0c0] 42     ==  [0x7ffefdb7f0e0] hello  ?   false
[0x7ffefdb7f0c0] 42     ==  [0x7ffefdb7f110] hello  ?   false
[0x7ffefdb7f0c0] 42     ==  [0x7ffefdb7f140] bye    ?   false
[0x7ffefdb7f0d0] 5      ==  [0x7ffefdb7f0b0] 42     ?   false
[0x7ffefdb7f0d0] 5      ==  [0x7ffefdb7f0c0] 42     ?   false
[0x7ffefdb7f0d0] 5      ==  [0x7ffefdb7f0d0] 5      ?   true
[0x7ffefdb7f0d0] 5      ==  [0x7ffefdb7f0e0] hello  ?   false
[0x7ffefdb7f0d0] 5      ==  [0x7ffefdb7f110] hello  ?   false
[0x7ffefdb7f0d0] 5      ==  [0x7ffefdb7f140] bye    ?   false
[0x7ffefdb7f0e0] hello  ==  [0x7ffefdb7f0b0] 42     ?   false
[0x7ffefdb7f0e0] hello  ==  [0x7ffefdb7f0c0] 42     ?   false
[0x7ffefdb7f0e0] hello  ==  [0x7ffefdb7f0d0] 5      ?   false
[0x7ffefdb7f0e0] hello  ==  [0x7ffefdb7f0e0] hello  ?   true
[0x7ffefdb7f0e0] hello  ==  [0x7ffefdb7f110] hello  ?   true
[0x7ffefdb7f0e0] hello  ==  [0x7ffefdb7f140] bye    ?   false
[0x7ffefdb7f110] hello  ==  [0x7ffefdb7f0b0] 42     ?   false
[0x7ffefdb7f110] hello  ==  [0x7ffefdb7f0c0] 42     ?   false
[0x7ffefdb7f110] hello  ==  [0x7ffefdb7f0d0] 5      ?   false
[0x7ffefdb7f110] hello  ==  [0x7ffefdb7f0e0] hello  ?   true
[0x7ffefdb7f110] hello  ==  [0x7ffefdb7f110] hello  ?   true
[0x7ffefdb7f110] hello  ==  [0x7ffefdb7f140] bye    ?   false
[0x7ffefdb7f140] bye    ==  [0x7ffefdb7f0b0] 42     ?   false
[0x7ffefdb7f140] bye    ==  [0x7ffefdb7f0c0] 42     ?   false
[0x7ffefdb7f140] bye    ==  [0x7ffefdb7f0d0] 5      ?   false
[0x7ffefdb7f140] bye    ==  [0x7ffefdb7f0e0] hello  ?   false
[0x7ffefdb7f140] bye    ==  [0x7ffefdb7f110] hello  ?   false
[0x7ffefdb7f140] bye    ==  [0x7ffefdb7f140] bye    ?   true
\$\endgroup\$
  • 1
    \$\begingroup\$ I rolled back your last edit. After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jan 14 at 18:03
  • \$\begingroup\$ The change comes with an "Edit" note that makes sure the reader follows + gives credit to the answer. \$\endgroup\$ – Amir Kirsh Jan 14 at 18:06
  • \$\begingroup\$ On hold as off topic? 😱 Not an intuitive use case?? 😲 -- Consider std::vector<unique_ptr<Shape>> and we want to check if a certain given Shape& is equal to a certain shape managed in the vector. \$\endgroup\$ – Amir Kirsh Jan 15 at 11:24
  • 1
    \$\begingroup\$ This question is being discussed on meta. \$\endgroup\$ – Mast Jan 16 at 14:52
  • 2
    \$\begingroup\$ "Edit" notes still violate site policy. This is Code Review, not Collaborative Coding. \$\endgroup\$ – 200_success Jan 16 at 23:28
3
\$\begingroup\$

First and foremost, I believe that your fundamental concept is flawed. Equality has a well defined meaning between objects of the same type, or between types themselves, but not in this hybrid scenario. For instance, let's say I have a class C, which inherits from A as well as from Base<C>, how shall I compare an instance of A and an instance of C? You would normally expect them to be comparable in a polymorphic context, at least if A implements operator==, wouldn't you? But their type_info is different. You could try and work around that problem by using dynamic_cast, but then I suspect that, given A a; C c;, a == c and c == a wouldn't return the same result, which is unexpected.

I suspect it's a kind of work-around you devised because you had a lot of vaguely related objects bunched together and realized afterwards you had to discriminate between them according to their concrete type.

But it would be best to improve on your overall design (that you can submit here, by the way), rather than persisting in the original one. Because what's the limit? Why not ordering objects of different types when you're at it? A common task when you have equality-comparable objects is to remove duplicates: you'll need to sort the objects to do it efficiently, meaning you have to implement operator< as well.

If you stick to your concept, though:

  • Your base class lacks a virtual destructor. Any base class that will be used as a polymorphic handle needs a virtual destructor, or it might lead to resources leak (see this).

  • The virtual print function clearly isn't in the right place (there's no conceptual link between equality and printability), but I suspect it's there only for the purpose of debugging.

  • The code seems to be over-engineered. The only thing your CRTP base class does is a static down-cast, which could be a nice (static polymorphism) if the base_equals method wasn't called after already having performed run-time type identification. It's simpler to directly dynamic_cast your pointers.

For instance:

class Base {
    public:
    virtual bool operator==(const Base&) =0;
    virtual ~Base() = default;
};

template <typename T>
class CRTP : public Base{
    virtual bool operator==(const Base& o) const override final {
        auto same_type = dynamic_cast<const T*>(&o);
        return same_type && static_cast<const T&>(*this) == *same_type;
    }
};
  • there's a balance issue in your code, because it will accept to compare objects of different derived types not implementing the equality operator, but will refuse to compare objects of the same type if it doesn't implement it. To remedy that issue you can check through SFINAE or concepts (C++20) if equality comparison is implemented. Or you can make a different trade-off and skip the CRTP step:

For instance:

class Base {
    public:
    virtual bool operator==(const Base& o) const { return this == &o; }
    virtual ~Base() = default;
};

class A : public Base {
    public:
    int i = 4;
    virtual bool operator==(const Base& o) const override final {
        auto same_type = dynamic_cast<const A*>(&o);
        return same_type && i == same_type->i;
    }
};

class B : public Base {}; 

The downside is that you have to implement the down-casting in each derived base class, and the upside that you have a more consistent fall-back scenario for derived classes not implementing an equality operator.

\$\endgroup\$
  • \$\begingroup\$ missing virtual dtor - oops - taken 👍; checking same_type with dynamic_cast is wrong it allows equality check between Person and Student on Person's fields -- the check with type_id is the correct way; print - it is of course for the sake of the question and not to present any specific design coupling; virtual operator== would take you to the derived class so you need to implement the "same_type" check in all derived classes... seems better to have non-virtual operator==; \$\endgroup\$ – Amir Kirsh Jan 14 at 18:12
-2
\$\begingroup\$

I apologize if I'm misunderstanding the problem your solving. If you're merely trying to first determine if two types are subclasses of a third, would it be possible to use avoid mixing an abstract base class and CRTP in favor of an operator overload like this? If so, you can get rid of the CRTP and the AbstractBase::base_equals method.

template< typename T, typename U >
bool operator==(T&& lhs, U&& rhs) { 
  return std::addressof(std::forward< T >(lhs)) == std::addressof(std::forward< U >(rhs)) ||
         std::is_base_of< AbstractBase, T >::value && std::is_base_of< AbstractBase, U >::value;
}

In Response to Original Poster's Reply

My earlier suggestion was simply (but not clearly stated) that you might pick either dynamic polymorphism or parametric polymorphism (not both). Also, I'm checking the memory addresses of objects to as a simple check of equality (to answer part of your reply).

Below is a solution I would use (using CRTP due to the different return types of getValue(); you could use dynamic polymorphism and achieve the same using operator<<). The hiccup is that you can no longer use std::vector, since you have pointers to both BaseTemplate<A>* and BaseTemplate<B>*, so you would need a heterogenous container in your driver code (ie std::tuple or the like).

Per the requirements in your reply, type/memory/value equality are checked with the free overloaded equality operators (you may need to decay the types if const/volatile cause types that fit your definition of equality don't bind to the correct operator== overload).

#include <iostream>
#include <type_traits>

template <typename Derived> class BaseTemplate {
  std::ostream &print(std::ostream &out) const {
    return static_cast<const Derived *>(this)->print(out);
  }

public:
  const auto &getValue() const {
    return static_cast<const Derived *>(this)->getValue();
  }
  friend std::ostream &operator<<(std::ostream &out,
                                  const BaseTemplate<Derived> &rhs) {
    return out << "[" << (void *)&rhs << "] " << rhs.getValue();
  }
  auto operator*() const { return *static_cast<const Derived *>(this); }
};

class A : public BaseTemplate<A> {
  int num;

public:
  A(int i = 42) : num(i) {}
  const int &getValue() const { return num; }
  bool operator==(const A &a) const { return num == a.num; }
};

class B : public BaseTemplate<B> {
  std::string str;

public:
  B(const std::string &s = "hello") : str(s) {}
  const std::string &getValue() const { return str; }
  bool operator==(const B &b) const { return str == b.str; }
};

template <typename T, typename U> bool operator==(const T &lhs, const U &rhs) {
  return false;
}

template <typename T> bool operator==(const T &lhs, const T &rhs) {
  return std::addressof(lhs) == std::addressof(rhs) || lhs == rhs;
}

\$\endgroup\$
  • \$\begingroup\$ there are two needs: (a) to check that the type is identical; (b) to check that the value is equal. Note that the required equality is for value and not for address. @Kedar - your proposed code does (a) but doesn't do (b). The CRTP in my original code is for (b). How would you do (b)? Do you have a version without CRTP? \$\endgroup\$ – Amir Kirsh Jan 14 at 7:43
  • \$\begingroup\$ Might be that you have an infinite recursion in your new solution? Operator == calling itself. \$\endgroup\$ – Amir Kirsh Jan 14 at 19:01
  • \$\begingroup\$ It didn’t happen when I tested it, but to be sure, you can change the lhs == rhs (within the method) to lhs.getValue() == rhs.getValue() \$\endgroup\$ – Kedar Bhat Jan 15 at 2:26
  • \$\begingroup\$ Interesting that it didn't recurse for you. Did you test it by actually running the code? Can you share a link to the code? And for the alternative of calling getValue() does it mean that any type has this method? Does it mean that your solution is mainly for types with single value that implements the method getValue? \$\endgroup\$ – Amir Kirsh Jan 15 at 4:30
  • \$\begingroup\$ I did run the code. Your point is valid; I should do the value comparison with (lhs|rhs).getValue(); it’s safer and is part of your requirement anyway. All parameter types of BaseTemplate should have getValue(), though the code above actually doesn’t enforce it properly (since I overloaded operator*, which yields a const reference to an instance of BaseTemplate parameter type). If by your question about whether the solution is for single types, it’s for determining if types are the same, and if so, if they hold the same value). Non-matching types hit the other overload (which yields false). \$\endgroup\$ – Kedar Bhat Jan 15 at 6:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.