0

I have an abstract class, IPacket.

/*
 * Represents an abstract network packet.
 */
class IPacket
{

public:

    virtual void read(...) = 0;

    virtual void write(...) = 0;

    virtual int getID() const = 0;
};

And I'd like to (and have been) returning a pointer to like so:

class PacketMedium
{
    /*!
     * Returns a pointer to the next pending packet or NULL if there are no packets pending.
     * \note The object must deleted when you are finished with it.
     */
    IPacket* receivePacket();
};

Now obviously, this is bad practice; requiring the caller to delete a pointer that wasn't even allocated by itself. The convention I believe, is to use smart pointers, i.e.

class PacketMedium
{
public:

    std::unique_ptr<IPacket*> receivePacket();
};

But as this is library code, smart pointers are a nono, notwithstanding the fact I'd prefer to avoid them regardless.

What would my best option be?

Thanks for any help :)

EDIT: this has been asked before and very good answers were given, although they all suggest smart pointers, or just not allocating on the heap. Given that IPacket is abstract, stack allocation wouldn't work.

Dylan
  • 572
  • 5
  • 10
  • I don't understand; either you want to ensure the caller `delete`s the object or you can use a smart pointer to manage it for you, however you don't seem to want either. So what do you want? – trojanfoe Jul 06 '13 at 09:28
  • Maybe he wants to return the object without a pointer of any kind. Compilers are good at copy elision these days. –  Jul 06 '13 at 09:30
  • I believe I have to use dynamically allocate `IPacket`'s because derived classes have different sizes so it would be impossible to use derivatives of `IPacket` while they are held on the stack. Smart pointers seem to be my only option. I'm looking for some way to restructure my code so that I can create IPacket objects and delete them in an obvious way. – Dylan Jul 06 '13 at 09:38
  • Why is this a bad practice? Who says so? – n. m. could be an AI Jul 06 '13 at 09:44
  • 1
    `Apart from the fact that many libraries were written before the advent of standard smart pointers, the biggest reason is probably the lack of a standard C++ ABI.` http://stackoverflow.com/questions/10334511/why-do-c-libraries-and-frameworks-never-use-smart-pointers – Dylan Jul 06 '13 at 09:48
  • my quote isn't very insightful but the answer is more so – Dylan Jul 06 '13 at 09:55
  • An alternative to dynamically allocating is to use the visitor pattern. – Vaughn Cato Jul 06 '13 at 11:16
  • 3
    @Dylan That’s a rotten reason. Good libraries use C++ types in their interfaces, a library that doesn’t do that is not a very good library. And the lack of standardised binary interface is less severe than you apparently think, since you just need to recompile the library for different architectures, and for both C and C++ that’s pretty common anyway. Summary: **do** use a smart pointer, it’s the proper solution here. – Konrad Rudolph Jul 06 '13 at 12:04
  • I think I'll give in and use `unique_ptr`, thanks for the information. – Dylan Jul 06 '13 at 23:45

3 Answers3

2

One idea is to return a reference:

class PacketMedium {
public:
   IPacket &receivePacket();
private:
   IPacketImpl1 impl1;
   IPacketImpl2 impl2;
 };

The receivePacket should be implemented in the following way:

IPacket &receivePacket() {
  int data = receiveint();
  if (data==0) { // impl1
      float data = receivefloat();
      impl1.data = data;
      return impl1;
  } else { // impl2
      std::string data = receivestr();
      impl2.str = data;
      return impl2;
  }
}

Note that there will be some ground rules when using the reference:

  1. Calling receivePacket() twice is dangerous. The 2nd call to it might erase existing data.
  2. The data you receive should be immediately used. Storing IPackets for longer time is dangerous.

To fix these problems, you could implement new function to IPacket interface:

virtual IPacket *clone() const=0;
tp1
  • 1,197
  • 10
  • 17
  • that's actually a very helpful answer, although I intend to wait for a bit to see if there's any better alternatives; the restrictions places (namely storing for longer times) is somewhat of a putoff. – Dylan Jul 06 '13 at 09:54
  • _'wait for a bit to see if there's any better alternatives'_ Don't really think so regarding the restrictions you give. I also don't see a reason, why the use of smart pointers in a library API should be bad. – πάντα ῥεῖ Jul 06 '13 at 11:16
0

You don't even need to return a reference. You can just wrap the public/relevant IPacket interface behind a class which also has advance operations and takes care of the deletion of incoming packets as they are consumed (i.e. your type calls receivePacket()).

If binary compatibility is your main concern, you can just write your own simple unique_ptr.

justin
  • 104,054
  • 14
  • 179
  • 226
0

Another solution is "handles". Instead of unique_ptr<IPacket> reveivePacket() you could wrap the unique_ptr to a type like this:

struct IPacketHandle { int id; };
IPacketHandle receivePacket();

But then where is packets stored? It would be inside std::vector<IPacket*> vec, but it's internal to your code. Then dovec.push_back(new IPacketImpl1); handle.id = vec.size()-1;. Note that deleting items from the vector might be more complicated/should replace the object with NULL pointer.

But wait, now you lost IPacket interface's read/write functions. Need to add them again:

void read_packet(IPacketHandle h, ...) { vec[h.id]->read(...); }
void write_packet(IPacketHandle h, ...) { vec[h.id]->write(...); }

(I've used this solution successfully in a library, and it gives nice functional programming interface similar to haskell's standard library, where there is no memory management for library users at all) (also the location of std::vector needs to be consiudered carefully, so that global variables dont need to be used)

tp1
  • 1,197
  • 10
  • 17