Check and Deletion not working as expected

Question

I have a templated class

template <class T>
class Node
{
    private:
        T data;

    public:
        Node<T> *next;
        Node(T initial);
        ~Node();
};

and the destructors are as follows:

template <class T>
Node<T>::~Node()
{
    if (std::is_pointer<T>::value)
    {
        delete this->data;
    }
}

template <class T>
LinkedList<T>::~LinkedList() {
    Node<T> *this_nodes = this->head;
    Node<T> *next = NULL;

    while (this_nodes)
    {
        next = this_nodes->next;
        delete this_nodes;
        this_nodes = next;
    }
}

However, when I attempt to compile with g++ with C++11, with the following driver code

LinkedList<int> list;
list.insert(5);
assert(list.read() == 5);

I get the following error:

error: type ‘int’ argument given to ‘delete’, expected pointer

The Node is the interior of the LinkedList, which I left off as it's not fully necessary to help answer this question. I'm not sure why this error appears as I have a check in place to assure the data is a pointer before attempting to delete it.

Can someone enlighten me a bit further on why this won't work / an alternative method? (NOTE: Cannot use smart pointers for this assignment)

`delete this->data;` - `data` is an `int`. Why are you trying to `delete` it ? The enshrouding `Node` is what should be deleted (and in fact, is being so, otherwise your destructor wouldn't be firing). If you're trying to disqualify something that isn't a pointer, the code itself should be omitted by something like SFINAE or specialization; not in a runtime if-block. — WhozCraig, Feb 25 '17 at 04:55
Why are you deleting `data` to begin with? Leave it alone. Your class shouldn't have any idea if the data was dynamically allocated. A pointer doesn't need to be allocated with `new`, so your pointer check isn't even warranted. — PaulMcKenzie, Feb 25 '17 at 05:09
The other parts of the driver user other elements like int*, Card* (A custom class), and a few others. The data in those doesn't get deleted with an empty constructor and I'm not sure how to free that memory appropriately. I know data is an int, and as such the if statement should fail and not do anything to it. It should only react when I get an actual pointer as the type (Node for example) — ZeldaZach, Feb 25 '17 at 05:16
I deleted my answer because it only showed you how to get rid of the compilation error but as suggested by others this isn't a right way to do it. [Here you can find an answer.](http://stackoverflow.com/questions/22847803/c-destructor-with-templates-t-could-be-a-pointer-or-not) — mpiatek, Feb 25 '17 at 05:20
@ZeldaZach -- If you have pointers, then again, your class should not care. If the user wants to introduce pointers to your class, then the user is responsible for clean up memory. If you want a concrete example, look at the STL classes such as `std::vector` or `std::list`. If the user wants to create a `std::vector`, then the `Card *` they put into the vector is the *user's* responsibility, not `vector`. The template class should not know or care where that pointer comes from. All the template class cares about is the memory needed to maintain it's data, nothing else. — PaulMcKenzie, Feb 25 '17 at 17:28

score 1 · Accepted Answer · answered Feb 25 '17 at 05:47

if statements are a tool to create runtime branches in your code. Not conditional compilation. When expanding your template code, it will look something similar to this:

Node<int>::~Node()
{
    if (false)
    {
        delete this->data;
    }
}

You are still deleting a int, even if it's inside a dead branch, the code must still be valid.

The solution should really to not touch the pointer. What do you expect this code to output?

struct A { ~A() { std::cout << "~A()" << std::endl; } };

auto a = new A;

std::vector<A*> vec;

vec.emplace_back(a);

vec.clear();

The answer is nothing. The vector will delete his buffer, but won't delete your data. If you want that to happen, use std::unique_ptr:

struct A { ~A() { std::cout << "~A()" << std::endl; } };

auto a = new A;

std::vector<std::unique_ptr<A>> vec;

vec.emplace_back(a);

vec.clear();

Now anyone will expect this code to output ~A(). The same goes with your code. Usually, you expect types to manage themselves and free any acquired resources. So the change in your code would simply be to drop the delete and assume the type to manage itself.

If you really want to do it anyway, which I really don't recommend, you can do this:

template<typename T>
void deleteIfPtr(T* ptr) { delete ptr; }

template<typename T>
void deleteIfPtr(const T&) { /* else, do nothing */ }

template <class T>
Node<T>::~Node()
{
    deleteIfPtr(this->data);
}

I decided to go with the unique_ptr, thanks for your assistance! — ZeldaZach, Feb 28 '17 at 06:37

score 0 · Answer 2 · answered Feb 25 '17 at 05:23

You can't use the logic

    if (std::is_pointer<T>::value)
    {
        delete this->data;
    }

The compiler will compile the line

        delete this->data;

regardless of the type used to instantiate Node.

You'll have to use something different. Example:

template <typename T> struct Deleter
{
   static void deleteObject(T) { /* Do nothing */ }
};

template <typename T> struct Deleter<T*>
{
   static void deleteObject(T* obj) { delete obj; }
};

template <class T>
Node<T>::~Node()
{
    Deleter<T>::deleteObject(data);
}

Check and Deletion not working as expected

2 Answers2