3

I'm trying to store references to objects that inherit from a nested abstract base class inside a std::set in the outer class.

Let me show you the code first since I think that's much more clear:

Class.h interface:

#ifndef Class_H_
#define Class_H_

#include <set>
#include <memory>

class Class
{
    public:
        class AbstractBase;
    private:
        std::set< std::shared_ptr<AbstractBase>> set;
    public:
        Class();
        ~Class();

        void add(AbstractBase& object);
};

#endif

Abstract.h interface:

#ifndef ABSTRACT_H_
#define ABSTRACT_H_

#include "Class.h"

class Class::AbstractBase
{
    friend class Class;

    public:
        virtual ~AbstractBase();
    private:
        virtual void foo(int in) = 0;
};

#endif

Derived.h interface:

#ifndef DERIVED_H_
#define DERIVED_H_

#include "Class.h"
#include "AbstractBase.h"

class Derived : private Class::AbstractBase
{
    public:
        ~Derived() override;
    private:
        void foo(int in) override;
};

#endif

add.cc implementation:

#include "Class.h"
#include "AbstractBase.h"

void Class::add(AbstractBase& object)
{
    // create a shared pointer to object
    // and insert it in the set
    set.insert(std::make_shared<AbstractBase>(object));  // <-- this is where it fails
}

So I would have multiple different derived objects all inheriting from AbstractBase which need to be stored together in the std::set container. Compilation fails because of the pure virtual function. At first, I didn't use the std::shared_ptr and thought that this was the reason for failure, I found this SO answer suggesting to use a std::shared_ptr instead. I'm still getting a compilation error in Class::add because AbstractBase::foo is pure, but I thought the std::shared_ptr would solve this?

I found similar questions but nothing that helped me solve my specific problem. Could someone explain to me what I'm doing wrong?

Thank you.

EDIT: Wow! Thanks for the informative answers, I'll need some time to thoroughly understand them all and see what works best for me. I'll update this question once I'm done!

  • 3
    "Could someone explain to me what I'm doing wrong?" your `add` function seems to have argument of wrong type (logically). It should be `add( std::shared_ptr obj )` instead. – Slava Jan 07 '20 at 14:29
  • Basically problem is that your `add()` method should take ownership of the object passed to it, though it is technically possible to do that with a reference it is not a proper solution. You need to transfer ownership by passing smart pointer. – Slava Jan 07 '20 at 14:39
  • Thanks, I tried different variations of what you suggested but this didn't compile either. Are you sure what you suggested is right? And yes, I'm still learning and reading a lot:) – pjterlustig Jan 07 '20 at 14:47
  • What variations? Change argument type to `std::shared_ptr` and insert that pointer directly without `std::make_shared` statement (better using `std::move`) – Slava Jan 07 '20 at 14:51
  • Should the container store the same object which is passed in (changing the object via the container will also change the original and vice versa), or should it store a new independent object (changing the object via the container or the original won't change the other)? – aschepler Jan 07 '20 at 14:53
  • What is the reason for using `std::set`? Do you want to prevent adding duplicates (if so, what determines whether two objects are the same)? Do you want to test for presence efficiently? Or something else? – Max Langhof Jan 07 '20 at 14:56
  • 1
    @Slava I'll look into your suggestion! @aschepler I do not want to create a new object. The intent is that when a certain event occurs (so I'll change the container to a `std::map>`) I iterate over the objs in the map and call all `foo()` functions of the objects in the container mapped to the event. @Max Langhof I need the set for the reasons described above, having an object in the container twice doesn't make sense which is why I opted for the `std::set`. – pjterlustig Jan 07 '20 at 15:55
  • @pjterlustig For the record, you cannot tag multiple people in the same comment - it won't notify them. Don't ask me why. – Max Langhof Jan 07 '20 at 16:05

3 Answers3

4

What your function attempts to do is make a copy of an object, allocate a shared instance for the copy, and store pointer to the copy.

Since your intention is to "store references" in the set, you presumably intend to store the objects elsewhere and don't actually want copies. If those referred objects are shared, then the correct solution is to pass the shared pointer:

void Class::add(std::shared_ptr<AbstractBase> ptr)
{
    set.insert(std::move(ptr));
}

If the referred objects aren't shared, then you cannot refer to them with a shared pointer. You can use a non-owning pointer instead:

std::set<AbstractBase*> set;

void Class::add(AbstractBase* ptr);

However, be very careful with the non-owning approach to keep the referred objects alive at least as long as they are referred by the set. This is non-trivial. Reference can be used as an argument to add, but I recommend against this, since it may not be obvious to the caller that the function will store pointer to the argument for longer than the functions execution.


If you do want to copy, then you can use a virtual function that returns a shared pointer. Example:

class Class::AbstractBase
{
    public:
        virtual std::shared_ptr<AbstractBase> copy() = 0;
// ...

class Derived : private Class::AbstractBase
{
    public:
        std::shared_ptr<AbstractBase> copy() override {
            auto ptr = std::make_shared<Derived>(*this);
            return {ptr, static_cast<Class::AbstractBase*>(ptr.get())};
        }
// ...

void Class::add(AbstractBase& object)
{
    set.insert(object.copy());

To avoid repeating the identical copy in multiple derived types, you can use CRTP.

eerorika
  • 232,697
  • 12
  • 197
  • 326
  • Thank you! Did a quick test and it worked. However, I would like to keep the signature as it is, this is part of a larger project with predefined signatures. The add() function receives a reference to derived objects which then need to be stored in a container in the outer class. – pjterlustig Jan 07 '20 at 14:53
  • @pjterlustig In that case consider using `std::ref` and storing the resulting `std::reference_wrapper`s. – Max Langhof Jan 07 '20 at 14:54
  • 1
    @pjterlustig You can use a reference argument for the non-owning approach if you want to, but I would recommend against storing references / pointers to arguments passed by reference. It should only be done in cases where it is obvious that the reference will be stored for longer than the duration of the function. Wrong assumption will lead to danging pointer / reference. – eerorika Jan 07 '20 at 14:57
2

If you want to copy a class of unknown dynamic type, there are three well-known ways to get around having insufficient information:

  1. Have a way to map the object to a handler expecting that specific most-derived type. typeid, a member-function, or a data-member in the common base-class which is its static type is most often used. This costs time, and is cumbersome to set up, but at least you often don't have to modify the class or use fat pointers.

  2. Have a function to .clone() the object in the statically known base. This is known as the virtual constructor idiom, and generally the most efficient and convenient to set up.

  3. Lug around an extra-pointer for cloning. This is the least invasive to the type or regarding additional setup, but changes the interfaces.

Which is most appropriate is yours to decide.
That is, if you actually want to copy the object, and shouldn't have passed a shared_ptr to .add() instead.

Deduplicator
  • 44,692
  • 7
  • 66
  • 118
  • Thanks for the information, not exactly what I'm looking for but since I'm new to C++ definitely interesting! As @Slava wrote, I do not want to make a copy, I'm looking for a way to store different references/pointers/smart pointers to derived objects in one container so that I can call their virtual function from `Class`. The `add()` function should receive a reference though. – pjterlustig Jan 07 '20 at 16:12
  • @pjterlustig it should not receive a reference if you do not want to make a copy. – Slava Jan 07 '20 at 16:18
2

You need to clarify the ownership of the objects stored in the set. If you use std::shared_ptr, you fundamentally encode that the set inside each Class owns the contained instances. This is incompatible with an add(AbstractBase&) method - you cannot really take ownership of an arbitrary object by reference. What if this object is already managed by a different shared_ptr?

Maybe you actually only want to store copies in the set. In that case, see the other answer(s) for ways to polymorphically copy ("clone") objects.

It is also open why you want to use std::set. std::set establishes uniqueness of the contained objects using the < operator (or a user-provided comparison functor with equivalent semantics). Do you even want uniqueness? If so, based on what criteria? Currently, there is no way to compare the stored class objects. std::shared_ptr "solves" that problem by instead comparing the internal pointer values, but I doubt that's what you need here.

If you actually want to store and compare objects solely based on their memory locations and not assume ownership of the stored objects, you could just use raw pointers. If you only want to store a whole bunch of objects without caring about uniqueness (since you currently attempt to create copies, each stored element would have a unique address and thus you currently would never use that aspect of std::set), maybe std::vector is the better solution. std::set may also help with determining whether an object is present in the collection efficiently, but std::vector can do that just the same (and possibly faster, if you sort and binary search). Consider the advice in http://lafstern.org/matt/col1.pdf.

Max Langhof
  • 23,383
  • 5
  • 39
  • 72
  • Instead of pointers, it may be more convenient to store `std::reference_wrapper` objects - that's still dependent on the actual owner's lifecycle management, of course. – Toby Speight Jan 07 '20 at 15:12
  • @Max Langhof Thanks for the explaination! I do not want to compare, so I'll look into using the `std::vector`. I chose a `std::set` because I want to iterate over the set and call each derived objs `foo()` function on a certain event. It doesn't make sense to do this twice therefore I opted for the set. – pjterlustig Jan 07 '20 at 16:01