0

I have a couple of problems in my code. The first is in the variable "Element" and it works well for me as long as the constructor of the class that sent the template its variables have default values, is there a way to skip the constructor without putting defaults values in the class? And the other problem is when it comes to freeing memory, when T is of the pointer type I will need to do the delete but just as I put the code I get an error, is there any other solution that can help me? I will be attentive to your answers, thanks: D

namespace Linked{
template <class T>
struct Nodo{
    const bool isponter = is_pointer<T>::value;
    T Element;
    Nodo<T> *Next;
    Nodo(){
        this->Next = nullptr;
    }
    ~Nodo(){
        if(is_pointer<T>::value)
            delete Element;
    }

};

}

NutCracker
  • 11,485
  • 4
  • 44
  • 68
  • 1
    What if the pointer is not to a dynamic object? You should not delete if `T` is a pointer. Like the standard collections, you should leave that responsibility to the original owner. – molbdnilo May 02 '20 at 07:56
  • 1
    You probably shouldn't be using delete at all but using unique_ptr or shared_ptr. See https://en.cppreference.com/w/cpp/memory/unique_ptr or https://en.cppreference.com/w/cpp/memory/shared_ptr – systemcpro May 02 '20 at 07:59

3 Answers3

1
namespace Linked{
template <class T>
struct Nodo{
    T Element;
    Nodo<T> *Next = nullptr;
    ~Nodo(){
        if constexpr (std::is_pointer<T>::value)
            delete Element;
    }
};

You should also consider if T is pointer to array.

273K
  • 29,503
  • 10
  • 41
  • 64
0

The only syntactial problems with your code are, that you didn't #include <type_traits> and forgot the std:: before is_pointer<T>::value.

But what you are attempting to do will raise problems with ownership. When Nodo contains a pointer it shouldn't own the object that pointer is pointing to. So you can't just delete that pointer, since you can't even know where it is pointing. Consider the following three cases, each of them requires different handling, but you have no way of determining what case you are facing:

Nodo<int*> n1, n2, n3;
n1.Element = new int(1); // requires delete

n2.Element = new int[10]; // requires delete[], crashes with delete

int i = 0;
n3.Element = &i; // no delete at all, crashes with delete

Usually whoever allocated a object on the heap is responsible for deallocating it. Nodo should not attempt to deallocate memory which it hasn't allocated.

Lukas-T
  • 11,133
  • 3
  • 20
  • 30
0

As you didn't specify a c++ version, I'm gonna assume you use the latest, which now is C++17. The closest fit to your existing code is using if constexpr, I won't elaborate on that as there are other good answers for that. If you are stuck on C++14 or C++11 (or worse 03/98, in which case you should simply upgrade), you will need to specialize your template. (I'll come back to this)

This code however, is violating one of the CppCoreGuidelines: ES.24: Use a unique_ptr<T> to hold pointers By writing your template to detect raw pointers and delete it, one always has to allocate. Hence your linked list can't refer to some sub-data of something existing. As already eluded to in the comments, if the users want the memory to be cleaned, use std::unique_ptr. An example:

namespace Linked{
  template <class T>
  struct Nodo{
    T Element;
    Nodo<T> *Next{nullptr};
    Nodo() = default;
    ~Nodo() = default;
  };
}

// Has ownership
auto node = Nodo<std::unique_ptr<int>>{};
node.element = std::make_unique<int>(42);

// Has ownership (to array of 42 elements)
auto node = Nodo<std::unique_ptr<int[]>>{};
node.element = std::make_unique<int[]>(42);

// No ownership
int value = 42;
auto node = Nodo<int>{};
node.element = &value;

With this, ownership is clear to the caller and transparant for you. (as you don't need to know about arrays, std::unique_ptr knows about that) You might want to put some restrictions on T, like adding static_assert(std::is_nothrow_move_constructable<T>);.

This above solution solves the problem in C++11 and upwards and should be the recommended approach.

If not, use if constexpr if your condition isn't capturable in a dedicated class in C++17. And partial specialization in C++14 and C++11.

namespace Linked{
  template <class T>
  struct Nodo{
    T Element;
    Nodo<T> *Next{nullptr};
    Nodo() = default;
    ~Nodo() = default;
  };
  template <class T>
  struct Nodo<T*>{
    T *Element{nullptr};
    Nodo<T> *Next{nullptr};
    Nodo() = default;
    ~Nodo() { delete Element; }
  };
}

If you don't want to repeat your code too much

namespace Linked{
  template <class T, class Me>
  struct AbstractNodo{
    T Element;
    Me *Next{nullptr};

    // All common code
  };
  template <class T>
  struct Nodo : AbstractNodo<T, Nodo<T>>{
    Nodo() = default;
    ~Nodo() = default;
  };
  template <class T>
  struct Nodo<T*> : AbstractNodo<T, Nodo<T*>>{
    Nodo() = default;
    ~Nodo() { delete Element; }
  };
}

There is also a way to specialize a single method, however, I'm not that familiar with it, see Stack overflow: Template specialization of a single method from a templated class for more details.

JVApen
  • 11,008
  • 5
  • 31
  • 67