Sign in to follow this  
Haytil

Is this code correct?

Recommended Posts

I'm having trouble with some really simple code, relating to dynamic allocation. I wanted to run it by someone else, to make sure I'm doing it correctly. You know how it goes, sometimes. Is this correct?

void function()
{
     int x = 5;

     // An array of classes
     CClass* m_Classes;
     m_Classes = new CClass[x];

     // An array of pointers to the classes
     CClass** m_Pointers;
     m_Pointers = new CClass*[x];

     // The ith elementh of the pointer array should point to the ith
     // element of the class array.  So the ith pointer points to the ith class

     for (int i = 0; i < m_Num_Live_Hornets; i++)
     {
          m_Pointers[i] = &m_Classes[i];
     }


     // .... do other stuff ....

     // Ok, now free all the memory we allocated

     if (m_Classes != NULL)
     {
          delete [] m_Classes;
          m_Classes = NULL;
     }

     if (m_Pointers != NULL)
     {
          delete [] m_Pointers;
          m_Pointers = NULL;
     }

}


Thanks. -Gauvir_Mucca

Share this post


Link to post
Share on other sites
What language? C++ I assume.

Is X==m_Num_Live_Hornets? If it is then remember to add an assertion both to show it to anyone reading your code and to let the compiler check that you don't have bugs If it isn't equal then you probably have an error since you create an array of X elements and iterate over the first m_Num_Live_Hornets elements.

A little thing to remember is to use prefix ++ instead of postfix ++ when you don't use the result. It will result in performance improvements for most types and improved clarity.

While everything you have is syntactically correct we can't be sure it does what you expect it to do until you tell us what it should do. Also how experienced are you with the SC++L and Boost? It might simplify some of the stuff you are doing.

EDIT: BTW you don't have to test for NULL when deleting.

Share this post


Link to post
Share on other sites
Your usage is correct: there are no memory leaks and the code is safe. This is, of course, assuming that the loop does copy over all addresses. I would probably have used this instead, though:


// A nice little trick from my personal library
template <typename T>
T* address_of(T& t) { return &t; }

// Your function
void function()
{
const int x;

std::vector<CClass> m_Classes(x);
std::vector<CClass*> m_Pointers(x);

std::transform(
m_Classes.begin(),
m_Classes.end(),
m_Pointers.begin(),
address_of<CClass>);

// Stuff

// RAII means no cleanup required
}



Share this post


Link to post
Share on other sites
Quote:
Original post by Gauvir_Mucca
I'm having trouble with some really simple code, relating to dynamic allocation. I wanted to run it by someone else, to make sure I'm doing it correctly. You know how it goes, sometimes.


Well, it will *work*, in the sense of compiling (modulo any dumb typos we overlooked) and not invoking undefined behaviour (unless m_Num_Live_Hornets > x), but these facts are a long way from "doing it correctly". ToohrVyk has lots of good ideas there. (Including the address_of thing - now that it's mentioned, I'm surprised that <functional> (or was it <numeric>?) doesn't include something like that along with std::plus, std::multiplies etc.)

But you should also consider:

- Your naming convention. Why is it vitally important, whenever you see the typename, to observe this 'C' tagging it as a concrete class rather than an interface? (I really hope that's why. If you're doing it to tag it as a class rather than a struct, then that is *absolutely useless* in C++; and if you're just doing it because you're following some guidelines in a book, then I don't know *what* to tell you.) If you use something incorrectly, the compiler will be more than happy to point out the problem PDQ, assuming you've designed things properly. Also, you haven't marked this as a member function of a class, but I assume it must be.

- What you're really trying to do. Why would you keep a separate container of pointers that all point to corresponding elements in the other container? It will be difficult to maintain if you change the contents, and you can trivially get one of those pointers any time you need it, by just taking the steps you did to create them in the first place. Don't cache redundant things except as a performance optimization (hint: looking something up in a container is not going to be a performance optimization versus a single address-of operator), and even then, Optimize Later(TM).

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this