c++ polymorphism and use of raw pointers

user_5095338 0
Viewed 106

I'm writing a packet processor and have the following classes:

class Parser {
  private:
    Packet* curr_pkt;
  public:
    void parsePacket(unsigned char byte) {
      if(byte == SyncPacket::headerVal) {
        SyncPacket pkt;
        curr_pkt = &pkt;
      }
      if(byte == SyncPacket::headerVal) {
        TypeAPacket pkt;
        curr_pkt = &pkt;
      }
    }
};
class Packet {
  public:
    void parseByte(unsigned char byte) {}
};
class SyncPacket : public Packet {
  public:
    static const unsigned char headerVal = 0x0A;
    void parseByte(unsigned char byte) {
      <do sync>
    }
};
class TypeAPacket : public Packet {
  public:
    static const unsigned char headerVal = 0x0B;
    void parseByte(unsigned char byte) {
      <do type A decode>
    }
};

int main() {
  std::vector<unsigned char> chars;
  <stream in characters>

  Parser pktParser;
  for(unsigned char byte: bytes) {
    pktParser.parseByte(byte);
  }
}

This seems to work fine and is in fact what I think polymorphism is for. But my question is: should I be worried about the raw pointer use here? Would there be a more recommended way to do this via smart pointers?

For example, in this line:

      if(byte == SyncPacket::headerVal) {
        SyncPacket pkt;
        curr_pkt = &pkt;
      }

Technically, pkt is out of scope after the if statement, so normally it's lifetime would be over. But because curr_pkt is pointing to it, it lives on. This is a potential issue, isn't it?

raw-pointer polymorphism c++
2 Answers

Technically, pkt is out of scope after the if statement, so normally it's lifetime would be over.

This is spot on correct, but there's nothing technically about it.

But because curr_pkt is pointing to it, it lives on.

This is not correct though, the object does not exist anymore at that point, the pointer is deemed dangling, and any attempt to dereference it would be Undefined Behavior.

If you managed to make this work, then you simply got lucky.

Would there be a more recommended way to do this via smart pointers?

Not so much a "more recommended" way, but a way that actually works, yes.

You can indeed use smart pointers to accomplish what your code is trying to express. (You don't need smart pointers to get there, but it does make it a lot safer).

class Parser {
  private:
    std::unique_ptr<Packet> curr_pkt;
  public:
    void parsePacket(unsigned char byte) {
      if(byte == SyncPacket::headerVal) {
        curr_pkt = std::make_unique<SyncPacket>();
      }
      if(byte == SyncPacket::headerVal) {
        curr_pkt = std::make_unique<TypeAPacket>();
      }
    }
};
Admin1

Yes this returning the address of a local variable is not going to work. And there is more to polymorphism, as explained in this example

#include <cstdint>
#include <memory>
#include <vector>

//-------------------------------------------------------------------------------------------------
// Packet is going to be an abstract base class 
// (closest thing C++ has to an interface)
class Packet_itf
{
public:
    // Dynamic polymorphism means virtual functions
    // for interfaces this will be pure virtual functions
    virtual void parseByte(unsigned char byte) = 0;

    // destructor needs to be virtual
    virtual ~Packet_itf() = default;

protected:
    // protected constructor Packet_itf is not supposed to be instantiated
    Packet_itf() = default;

    // todo delete copy/move/assignment rule of 5
};

//-------------------------------------------------------------------------------------------------
// define a null strategy for the interface
// an implementation that does nothing

class NullPacket :
    public Packet_itf
{
    void parseByte(unsigned char byte) override
    {
    }
};

//-------------------------------------------------------------------------------------------------
// here are your packets

class SyncPacket :
    public Packet_itf
{
public:
    static const std::uint8_t headerVal = 0x0A;

    void parseByte(unsigned char byte) override
    {
        //<do sync>
    }
};

//-------------------------------------------------------------------------------------------------

class TypeAPacket :
    public Packet_itf
{
public:
    static const std::uint8_t headerVal = 0x0B;

    void parseByte(unsigned char byte) override
    {
        //<do type A decode>
    }
};

//-------------------------------------------------------------------------------------------------
// Dynamic polymorphism renamed parser to factory
// factory pattern

class PacketFactory
{
public:

    static std::unique_ptr<Packet_itf> createPacket(const std::uint8_t byte)
    {
        switch (byte)
        {

        case SyncPacket::headerVal:
        {
            return std::make_unique<SyncPacket>();
        }
        break;

        case TypeAPacket::headerVal:
        {
            return std::make_unique<TypeAPacket>();
        }
        break;

        default:
            // error handling here
            break;
        }

        // return a default packet that does nothing
        // Null strategy pattern
        return std::make_unique<NullPacket>();
    }
};

//-------------------------------------------------------------------------------------------------

int main()
{
    //<stream in characters>
    // note 0x02 isn't know yet so ut will result in a NullPacket
    // no ifs in the loop need to handle that case.

    std::vector<std::uint8_t> bytes{ SyncPacket::headerVal, TypeAPacket::headerVal, 0x02 };


    for (const auto byte : bytes)
    {
        // now packet is a unique ptr that will live for the lifetime
        // of this for loop then it will be deleted.
        auto packet = PacketFactory::createPacket(SyncPacket::headerVal);

        std::vector<std::uint8_t> data{ 0x00,0x00,0x01,0x03 };
        for (const auto value : data)
        {
            packet->parseByte(value);
        }
    }

    return 0;
}
user_166495500
Related