Correct way to design data class to handle polymorphic data

Question

I need to design a struct data which will hold pointer to Base data type. User should be able to easily create object of this data struct and pass around without handling much of memory management issues.

I have created few structures, please suggest the correct way to handle it.

struct BaseData {
   enum DataType { DATATYPE_1, DATATYPE_2 };
   virtual ~BaseData() { cout << "BaseData Dtor" << endl; }
};

struct DataType1 : BaseData {
   virtual ~DataType1() { cout << "DataType1 Dtor" << endl; }
};

struct DataType2 : BaseData {
   virtual ~DataType2() { cout << "DataType2 Dtor" << endl; }
};

struct Data {
   Data() { cout << "Data Ctor" << endl; }
   Data(const Data& o) {
      if (o.baseData->type == BaseData::DATATYPE_1) { 
        baseData = new DataType1; 
        *(static_cast<DataType1*>(baseData)) = *(static_cast<DataType1*>(o.baseData)); 
      }
      else if (o.baseData->type == BaseData::DATATYPE_2) { 
        baseData = new DataType2; 
        *(static_cast<DataType2*>(baseData)) = *(static_cast<DataType2*>(o.baseData)); 
      }
   }
   virtual ~Data() { 
      cout << "Data Dtor" << endl; 
      delete baseData;  //here it results in segmentation fault if object is created on stack. 
      baseData = NULL; 
   }

   BaseData* baseData;
};

vector <Data> vData;
void addData(const Data& d) { cout << "addData" << endl; vData.push_back(d); }

The client code looks like below.

int main()
{
   {
      DataType1 d1;
      d1.type = BaseData::DATATYPE_1;
      Data data;
      data.baseData = &d1;      
      addData(data);
   }

   {
      BaseData* d2 = new DataType2;
      d2->type = BaseData::DATATYPE_2;
      Data data;
      data.baseData = d2;
      addData(data);
      delete d2;
      d2 = NULL;
   }

   {
      Data data;
      data.baseData = new DataType1;
      static_cast<DataType1*>(data.baseData)->type = BaseData::DATATYPE_1;
      addData(data);
      delete data.baseData;
      data.baseData = NULL;
   }
}

Code in block 1 and block 2 crashes due to double deletion. How can i handle all these use cases properly.

One way what I have thought of is, hide baseData pointer using private and provide a method to user setBaseData(const BaseData& o) in struct Data.

void setBaseData(const BaseData& o) { 
    cout << "setBaseData" << endl;
    if (o.type == BaseData::DATATYPE_1) { 
        baseData = new DataType1; 
        *(static_cast<DataType1*>(baseData)) = static_cast<const DataType1&>(o); 
    }
    else if (o.type == BaseData::DATATYPE_2) { 
        baseData = new DataType2; 
        *(static_cast<DataType2*>(baseData)) = static_cast<const DataType2&>(o); 
    }
}

With setBaseData() I am able to avoid segmentation fault and user is free to create object of struct Data in which ever he likes.

Is there any better way to design these classes?

score 1 · Answer 1 · edited Jun 20 '20 at 09:12

Code in block 1 and block 2 crashes due to double deletion. How can i handle all these use cases properly.

By following the rule of 3 (or rule of 5 if you would like to support efficient move operations):

if a class defines one (or more) of the following it should probably explicitly define all three:

destructor

copy constructor

copy assignment operator

You've neglected implementing a custom copy assignment operator. The use of the default copy assignment operator results in the double deletion.

Also, never assign a pointer to an automatic variable to Data::baseData like you do here in block 1.

The destructor of Data will delete this pointer, which results in undefined behaviour.

Also, never delete the pointer owned by Data::baseData unless you're going to replace it with something else.

To avoid doing these by accident, I recommend declaring Data::baseData private as you've already considered.

Is there any better way to design these classes?

Yes. Don't ever use bare pointers to owned memory. Use std::unique_ptr instead.

Daniel Trugman · Accepted Answer · 2017-10-04T23:39:25.007

Your problem is that you are trying to manage ownership by yourself. Instead you could use explicit ownership management using the unique_ptr type.

Assuming the same type definitions you used (+ The createDataType method we'll see later):

struct BaseData {
  enum DataType { DATATYPE_1, DATATYPE_2 };
  virtual ~BaseData() { cout << "BaseData" << endl; }

  static std::unique_ptr<BaseData> createDataType(DataType type);
};

struct DataType1 : BaseData {
  virtual ~DataType1() { cout << "DataType1" << endl; }
};

struct DataType2 : BaseData {
  virtual ~DataType2() { cout << "DataType2" << endl; }
};

Notice that we are now using a factory for creating our objects, like so:

static std::unique_ptr<BaseData> BaseData::createDataType(BaseData::DataType type) {
  switch(type) {
    case BaseData::DATATYPE_1:
      return std::make_unique<DataType1>();
    case BaseData::DATATYPE_2:
      return std::make_unique<DataType2>();
    default:
      throw std::runtime_error("ERR");
  }
}

Then, you should declare your managing Data object as follows:

struct Data {
  Data()
    : baseData(nullptr) {}
  Data(std::unique_ptr<BaseData> data)
    : baseData(std::move(data)) {}
  Data(Data && rhs)
    : baseData(std::move(rhs.baseData)) {}

  std::unique_ptr<BaseData> baseData;
};

And now we could write clean, clear and safe code as this:

vector<Data> vData;
void addData(Data&& d) {
  if (dynamic_cast<DataType1 *>(d.baseData.get()) != nullptr)
    cout << "Adding DataType 1" << endl;
  else if (dynamic_cast<DataType2 *>(d.baseData.get()) != nullptr) 
    cout << "Adding DataType 2" << endl;

  vData.push_back(std::move(d));
}

int main()
{
   { // Option 1: Create base data somewhere, create data from it
      auto baseData = createDataType(BaseData::DATATYPE_1);
      Data data { std::move(baseData) };
      addData(std::move(data));
   }

   { // Option 2: Create data directly knowing the base data type
      Data data { createDataType(BaseData::DATATYPE_2) };
      addData(std::move(data));
   }

   { // Option 3: Create data and add it to the vector
      addData({ createDataType(BaseData::DATATYPE_1) });
   }
}

And you could always check for the actual type of the baseData using the same dynamic casts as in addData

Correct way to design data class to handle polymorphic data

2 Answers2