Would this be considered good C++ code

Question

I have a vector with raw pointers (no, I cannot use smart pointers) and I want to add items to the list in a for loop. I've made a little trial project, and I wondered if this is considered good C++ code in terms of pointer management.

Please only consider raw pointer management, I am not interested in smart pointers for this particular problem I'm trying to solve.

A simple object:

class Request
{
public:
    std::string name;

};

std::vector<Request*> requests;

for (int i = 0; i < 5; i++)
{
    std::stringstream ss;
    ss << "elemenent ";
    ss << i;

    std::string s = ss.str();

    Request* req = new Request();   
    req->name = s;

    requests.push_back(req);

}

EDIT:

So the problem I am trying to solve is adding the DOMNode* to a vector from this library.
I'm starting to get the feeling that trying to write a wrapper for the parts I need from this library for my project, is a bad idea. Or maybe the library is no good? I haven't got it to work properly using smart_ptr, if anybody out there has, then I'd like to hear about it.

Answer 1

Well, this leaks memory, so it is bad. Can you use a Pointer Container?

The reason this code leaks is because you create objects on the heap using new, but you never call delete on them.

As for you comment, if you have an object that manually manages some resource, you need The Big Three.

Answer 2

If you are not able (or allowed) to use smart pointers, probably you could make use of a simple memory manager like this:

template <class T>
class MemManager
{
public:
  typedef std::vector<T*> Vec;
  ~MemManager ()
  {
    size_t sz = v_.size ();
    for (size_t i = 0; i < sz; ++i)
      delete v_[i];
  }
  T* pushNewObject () 
  {
    T* t = NULL;
    try
    {
        t = new T;
        if (t != NULL)
           v_.push_back(t);            
    }
    catch (std::bad_alloc& ex) { /* handle ex */ }
    return t;
  }
  const Vec& objects() const { return v_; }
private:
  Vec v_;
};

// test
{
  MemManager<Request> mm;
  for (int i = 0; i < 5; i++)
    {
      std::stringstream ss;
      ss << "elemenent ";
      ss << i;

      std::string s = ss.str();

      Request* req = mm.pushNewObject();
      req->name = s;    
    }
} // all Request objects will be deleted here when 
  // the MemManager object goes out of scope.

Answer 3

I'll consider that you have a loop, at the end of the method, to call delete on each member of the vector.

There are still issues, specifically exception safety issues.

• If anything throws between the creation of the Request and its registration in the vector, you've lost the memory. One solution is to temporarily use a scoped_ptr to hold on the memory, push_back with ptr.get() and then call the release method since now the memory is owned by the vector.
• If anything throws between the point when you have created the items in the vector and the point you destroy them, you need to catch the exception, destroy the items, and then rethrow.

There might be others, but RAII has been invented for a reason, it's really difficult to do without (correctly...)

Answer 4

If you cannot use smart pointers, then use boost::ptr_vector.

Note that if you are using TinyXml, memory management in XmlNode is probably dictated by the library - recent history iirc is that many of your problems are associated with properly understanding the memory ownership and release paradigm for this library.

What memory management do I need to cleanup when using TinyXml for C++?

What is the best open XML parser for C++?

Answer 5

A quick improvement could be to derive a class RequestVector from std::vector<Request*>, add a ClearRequests method (which deletes all the Request objects and clears the vector) and and make it's destructor call ClearRequests. (Actually aggregating the vector in RequestVector could be a better choice, but a derived class is faster done).