In this example, what is the safest way to delete the data variable?

Question

#include <iostream>
#include <cstdlib>

template<class To, class From>
To any_cast(From v)
{
    return static_cast<To>(static_cast<void*>(v));
}

class Something
{
public:
    template<class T>
    Something(T mydata);
    template<class T>
    void getData(T& t);
    ~Something();
private:
    unsigned char* data;
};

int main()
{
    {
        Something s(20.1);
        double ret = 0;
        s.getData(ret);
        std::cout << ret << std::endl;
    }
    return 0;
}

template<class T>
Something::Something(T mydata)
{
    data = new unsigned char[sizeof(mydata)];
    unsigned char* temdata = any_cast<unsigned char*>(&mydata);
    for (unsigned int i = 0; i < sizeof(mydata); i++)
    {
        data[i] = temdata[i];
    }
}

Something::~Something()
{
    delete[] data;
}

template<class T>
void Something::getData(T& t)
{
    T* tt = any_cast<T*>(data);
    t = *tt;
}

In the above example I take an object and turn it into an unsigned char array, or a byte array. Afterwords when the class deconstructor is called I delete this variable. However I seem to keep hitting breakpoints in my program when I delete the object "data".

I assume this is due to it thinking its a normal char* and looking for the terminating /0 to denote the end of the string.

I know since I'm allocating data i need to DE-allocate it, so what is the safest way to do so. Or is it even necessary?

Thought about using a std::vector and that would solve the problem. But I don't want the performance hit on data retrieval if the object is scattered across the heap.

I also tried both delete operators. I get a:

HEAP[test.exe]: Invalid address specified to RtlValidateHeap( 01380000, 04574468 )

Error with either one. (Sorry the delete was a typo)

The code is simplified from the original code, so I did not include the copy constructors and ext.

After seeing usage of std::any I tried it out in a test enviroment: Here and that seems to work alot better then my idea. But now I need to figure out how to make visual studios use a higher level compiler with access to c++17 features =/

out of curiosity: why are you doing this? I mean if I can only get or set a `T` from `Something`, then why does it internally not simply store a T ? — 463035818_is_not_an_ai, Jul 24 '17 at 19:29
Knee jerk answer [is with RAII](http://en.cppreference.com/w/cpp/language/raii) via `std::vector` — user4581301, Jul 24 '17 at 19:29
And you're breaking the [rule of 3](http://en.cppreference.com/w/cpp/language/rule_of_three). — HolyBlackCat, Jul 24 '17 at 19:35
Possible duplicate of [delete vs delete\[\] operators in C++](https://stackoverflow.com/questions/2425728/delete-vs-delete-operators-in-c) — Justin, Jul 24 '17 at 19:39
@HolyBlackCat Dident include it in the sample code due to thinking it was unnecessary for the question. — Steven Venham, Jul 25 '17 at 11:57
@tobi303 Example is a simplified version of my message queuing system. — Steven Venham, Jul 25 '17 at 12:08

Remy Lebeau · Answer 1 · 2017-07-25T01:38:33.337

data is allocated using new[], so your destructor needs to call delete[] instead of delete.

You are also missing a copy constructor and copy assignment operator, so your class does not satisfy the requirements of the Rule of Three, and thus can corrupt memory, cause crashes, etc.

Try something more like this:

#include <algorithm>

class Something
{
public:
    template<class T>
    Something(const T &mydata);

    Something(const Something &src);

    template<class T>
    void getData(T& t) const;

    ~Something();

    Something& operator=(const Something &src);

private:
    unsigned char* data;
    size_t data_size;
};

template<class T>
Something::Something(const T &mydata)
{
    data_size = sizeof(T);
    data = new unsigned char[data_size];
    const unsigned char* temp = reinterpret_cast<const unsigned char*>(&mydata);
    std::copy(temp, temp + data_size, data);
}

Something::Something(const Something &src)
{
    data_size = src.data_size;
    data = new unsigned char[data_size];
    std::copy(src.data, src.data + data_size, data);
}

Something::~Something()
{
    delete[] data;
}

template<class T>
void Something::getData(T& t) const
{
    const T* tt = reinterpret_cast<const T*>(data);
    t = *tt;
}

Something& Something::operator=(const Something &src)
{
    if (&src != this)
    {
        Something temp(src);
        std::swap(temp.data, data);
        std::swap(temp.data_size, data_size);
    }
    return *this;
}

Better would be to use std::vector instead of new[], and let the compiler and STL handle memory management for you:

#include <algorithm>
#include <vector>

class Something
{
public:
    template<class T>
    Something(const T &mydata);

    Something(const Something &src);

    template<class T>
    void getData(T& t) const;

private:
    std::vector<unsigned char> data;
};

template<class T>
Something::Something(const T &mydata)
    : data(sizeof(T))
{
    const unsigned char* temp = reinterpret_cast<const unsigned char*>(&mydata);
    std::copy(temp, temp + sizeof(T), &data[0]);
}

Something::Something(const Something &src)
    : data(src.data)
{
}

template<class T>
void Something::getData(T& t) const
{
    const T* tt = reinterpret_cast<const T*>(&data[0]);
    t = *tt;
}

Now, that being said, just know that this kind of coding is not safe when T is a non-POD type, specially if it has non-trivial constructors and assignment operators implemented.

It looks like you are trying to implement something similar to std::any, which requires a bit more work to support non-POD types safely.

Diden't want to use vectors due to the possibility of the data becoming scattered across the heap. It dosen't matter to me if the creation of the data is slow in my program. But the retrieval MUST be fast and I'm not sure a vector would be fast enough if the held data is not concurrent. — Steven Venham, Jul 25 '17 at 12:14
@StevenVenham `std::vector` is just a wrapper for `new[]` and `delete[]`, there is no difference in speed, heap usage, data access, etc, but `std::vector` follows proper RAII and Ruleof3 semantics so it is safer to use. — Remy Lebeau, Jul 25 '17 at 15:20
So if i waned to use it with forinstance strings and stuff would it be better to use a std::any as my input type and storage? — Steven Venham, Jul 25 '17 at 16:20
@StevenVenham what are you really trying to accomplish in the first place? — Remy Lebeau, Jul 25 '17 at 17:10
Store a variable of multiple types in a message. Store that in a queue. then later dispense those items to other classes for processing. — Steven Venham, Jul 26 '17 at 07:37
@StevenVenham I wouldn't use this approach for that purpose. I would use polymorphic classes instead. — Remy Lebeau, Jul 26 '17 at 07:43

In this example, what is the safest way to delete the data variable?

1 Answers1