Solved

memory/pointer question

Posted on 2014-10-17
12
126 Views
Last Modified: 2014-12-06
I'm trying to do some beginner learning here and just wanted to see if someone can mentor me on this matter. Are the two "delete"s in here needed?  And why or why not? And as a side if there's anything obviously wrong with my code please let me know.

Thanks!


typedef struct {
      uint8                  m_nodeId;
      list<OValue>      m_values;
} NodeInfo;
static list<NodeInfo*> g_nodes;

typedef struct {
      int                        time;
      OValue*                  m_value;
} ValueChange;
static list<ValueChange*> g_changes;


// deletes one of the elements from the array of ValueChange's if the OValue is in that element
void function DeleteChange(OValue v){

    for (list<ValueChange*>::iterator it = g_changes.begin(); it != g_changes.end(); ++it)
    {
        ValueChange* change = *it;
        OValue* v_i = change->m_value;

        if (v_i->GetId() == v.GetId())
        {
            g_changes.erase(it);
            delete v_i;
            delete change;
        }
    }

}
0
Comment
Question by:rustycp
[X]
Welcome to Experts Exchange

Add your voice to the tech community where 5M+ people just like you are talking about what matters.

  • Help others & share knowledge
  • Earn cash & points
  • Learn & ask questions
12 Comments
 
LVL 86

Accepted Solution

by:
jkr earned 500 total points
ID: 40387663
Yes, the two 'delete' operations are absolutely necessary, since you would create memory leaks if you omitted them. You could elimiate the last of the two by providing a destructor to 'ValueChange' that takes care of that, e.g.

typedef struct {
      ~ValueChange() { if (m_value) delete m_value;}
      int                        time;
      OValue*                  m_value;
} ValueChange;

Open in new window


That's the downside of using STL containers with pointers. If possible, try to avoid such situations. But, sometimes that simply is not possible...
0
 
LVL 32

Expert Comment

by:phoffric
ID: 40387843
>> if there's anything obviously wrong with my code please let me know.
Without seeing a small buildable program, it's hard to say whether anything is obviously wrong. For example, I don't know how g_nodes is used. And I don't know how your constructors and destructors are defined.

Another downside of using pointers (whether in STL containers or not) is that it is possible that the pointers are not pointing to objects that were dynamically allocated with the new operator; in that case, using delete would be wrong. Also, a pointer may be pointing to objects created with the new[] operator, in which case, you would need to use the delete[] operator.
0
 
LVL 32

Expert Comment

by:phoffric
ID: 40387849
Another concern, in general, is whether you are deleting objects that another object is still referencing.
0
Independent Software Vendors: We Want Your Opinion

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

 
LVL 2

Author Comment

by:rustycp
ID: 40387975
So here's what I'm not quite getting. I thought that since I declared those two pointer variables inside this function, that they only exist inside the function and would ni longer exist when the function exits. For that reason I don't understand why the delete is needed. I realize the OValue object that the ValueChange object points to will still exist because it is still pointed to inside one of the NodeInfos in the NodeInfo list, and thats ok because I want to keep it there. But I do want to remove that one ValueChange object from the ValueChange list and not have it taking up memory anymore. In other words since I removed the only reference to the ValueChange obj by using erase(it) from the ValueChange list, plus when the function exits and the local "change" pointer goes out of scope, then the ValueChange object should be able to be  "garbage collected"?
0
 
LVL 2

Author Comment

by:rustycp
ID: 40387979
To phoffrics last point - I do NOT want to delete the OValue object that the ValueChange object it pointing to. I use that elsewhere. So I should not do "delete v_i"? Even though v_i is a pointer and not the actual object?
0
 
LVL 32

Expert Comment

by:phoffric
ID: 40388031
Both g_nodes and NodeInfo are not used in your OP. If you post a short buildable program (and preferably one that gives correct results), then it will be easier to address your comments.
0
 
LVL 2

Author Comment

by:rustycp
ID: 40388619
g_nodes is a list of 10 NodeInfo. Inside each NodeInfo is a list of 10 OValue. So there are 100 OValue. This never changes.

Occasionally one of the 100 OValues changes, and a handler function creates a new ValueChange object with a pointer to the changed OValue in it as described and adds that to the ValueChange list. The program later comes back and removes that ValueChange from the list by calling this function, giving only the OValue object that has changed.

Hopefully that describes that there will always be the same 100 OValues objects, but a changing number of ValueChange objects. In that function I have a call to delete both pointers to a OValue and a ValueChange. I guess in particular I'm not clear why the call to delete the ValeChange pointer is needed? Or is it implicit when the function ends? Or will I create a memory leak without it?

I'll also try to write this into a basic program tomorrow.
0
 
LVL 32

Expert Comment

by:phoffric
ID: 40389344
There may be some missing details, but roughly speaking, if you have a new Ovalue for a key, ID, I am not sure why you are not replacing it in the list rather than doing any deletion. Your OP can be interpreted in different ways, and it is focused primarily on deletion rather than replacement.

If m_value is actually pointing to an OValue which is in the m_values list of Ovalue, then since you say that you always have a list of 10 Ovalue entries in each NodeInfo struct, then deleting it is not right. You could delete it and then insert the new Ovalue entry back into the list; but replacement seems like an easier option. (And I am assuming that this is single-threaded.)

Your comment reads
>> deletes one of the elements from the array of ValueChange's ...
yet your structures are lists. I am sure that this will be cleared up when you write a very small program to illustrate your points.
0
 
LVL 32

Expert Comment

by:phoffric
ID: 40389351
Your current description of your question seems to me to be very different from your OP description. I suggest that you close this question, and open a new one with your first attempt at solving the problem.

If possible, in the new question, please include a link or a verbatim description of the exercise that you are trying to solve to avoid any possible misinterpretations on our part.

(BTW, on weekends, I sometimes have time for a program type question; during the week, other experts will be able to help you at greater length.)
0
 
LVL 34

Expert Comment

by:sarabande
ID: 40392450
since I removed the only reference to the ValueChange obj by using erase(it) from the ValueChange list, plus when the function exits and the local "change" pointer goes out of scope, then the ValueChange object should be able to be  "garbage collected"?
c and c++ don't have garbage collection beside you would create smart pointers which are objects managing a pointer.

so, generally if you create a pointer with new (on the heap), you have to delete it. otherwise there is a memory leak (see comment of jkr).

g_changes.erase(it);
in your erase loop is a bug. after erasing the iterator it is no longer valid and because you didn't break from loop, the ++it at begin of next loop cycle would operate with an invalid iterator. if using a vector, only the next item after erasing was skipped. but if you erase the last item, the ++it statement will crash. for a list which chains items by pointer, it most likely will crash immediately at ++it. to come out of it, you may do:

list<ValueChange*>::iterator itnext; 
 for (list<ValueChange*>::iterator it = g_changes.begin(); it != g_changes.end(); it = itnext)
     {
         ValueChange* change = *it;
         OValue* v_i = change->m_value;

         if (v_i->GetId() == v.GetId())
         {
             itnext = g_changes.erase(it);  // erase returns a reference to the next iterator
             delete v_i;
             delete change;
         }
         else 
         {
              itnext = ++it;  // increment
         }    
     }

Open in new window


generally, you should only use pointers in collections, if the pointers are baseclass pointers for virtual use, or if you have more than one collection using the same objects, for example a vector for indexed access and a map for access by key.

if you do so, one of the collections must made responsible for deletion, while the other collection(s) may not delete theirselves. you would have all collections using the same set of pointers as members in one class and use the destructor to safely delete the pointers:

class X
{
     std::vector<Data*> v;
     std::map<std::string, Data*> m;
     std::list<Data*> l;
public:
     CreateData(int i, std::string k) 
     {
            v.push_back(new Data(i, k);
            m[k] = v.back();
            l.push_back(v.back());
       } 
     ~X()
     {
          for (size_t n = 0; n < v.size(); ++n)
               delete v[n];
          // just for readability ...
          v.clear();
          m.clear();
          l.clear();
      }
};

Open in new window


Sara
0
 
LVL 2

Author Comment

by:rustycp
ID: 40396737
Thanks man, this pointed me in the direction I needed to understand what the problems are that I'd had to deal with
0
 
LVL 32

Expert Comment

by:phoffric
ID: 40446053
You awarded all the points to my post http:#a40389351 and since that was not an answer, it was unaccepted. Since you have expressed satisfaction, it is time for you to accept one or more posts that have helped you.
0

Featured Post

Industry Leaders: We Want Your Opinion!

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

IntroductionThis article is the second in a three part article series on the Visual Studio 2008 Debugger.  It provides tips in setting and using breakpoints. If not familiar with this debugger, you can find a basic introduction in the EE article loc…
Basic understanding on "OO- Object Orientation" is needed for designing a logical solution to solve a problem. Basic OOAD is a prerequisite for a coder to ensure that they follow the basic design of OO. This would help developers to understand the b…
The goal of the tutorial is to teach the user how to use functions in C++. The video will cover how to define functions, how to call functions and how to create functions prototypes. Microsoft Visual C++ 2010 Express will be used as a text editor an…
The viewer will learn how to use the return statement in functions in C++. The video will also teach the user how to pass data to a function and have the function return data back for further processing.

688 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question