🔒 Locked

Is this code correct?

Started by Haytil Mar 7, 2007 at 4:44 AM 3 replies 900+ views

Original Post

Mar 7, 2007 at 4:44 AM

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 = &m_Classes;
     }


     // .... 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

CTar

Mar 7, 2007 at 4:53 AM

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.

ToohrVyk

Mar 7, 2007 at 5:55 AM

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 librarytemplate <typename T> T* address_of(T& t) { return &t }// Your functionvoid 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}

Zahlman

Mar 7, 2007 at 6:14 PM

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 (or was it ?) 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).

Nathan Baum

Mar 7, 2007 at 6:19 PM

Quote:
Original post by Gauvir_Mucca
...trouble...

But without knowing what the trouble is, it's hard to diagnose a likely cause.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.