std::list iterator

Question

The following code instead of returning a pointer back to an audioResource it returns something else which is invalid, i've gone through with a debugger and the problem is with this line

return *list_it;

Here is my function:

AudioResource* AudioManager::getResource(const unsigned int ID)
{
    std::list<AudioResource*>::iterator list_it;
    for(list_it = m_resources.begin(); list_it!= m_resources.end(); list_it++)
    {
        if((*list_it)->getID()==ID)
            {
std::cout<<*(*list_it)->getFileName();
            return *list_it;
        }
    }
    return nullptr;
}

O and I have tried putting it as (*list_it) but i got the same results =s

How it is populated...

Resource* AudioManager::addResource(const unsigned int ID, 
      const std::string fileName,  const unsigned int scope,
      const std::string type)
{
     AudioResource* temp;
     if(type == "AUDIO_TYPE_SAMPLE")
     {
          temp = new AudioResource(ID,fileName,scope,
                      RESOURCE_AUDIO,AUDIO_TYPE_SAMPLE);
          m_resources.push_back(temp);
     }
     else if(type == "AUDIO_TYPE_STREAM")
     {
          temp = new AudioResource(ID,fileName,scope,
                    RESOURCE_AUDIO,AUDIO_TYPE_STREAM);
          m_resources.push_back(temp);
     }

     return temp;
}

call to get resource

cout<<AudioManager::getInstance()->getResource(IDnum)->getFileName();

Design thoughts: Why a `list` not a `vector`? Or a `map`? Why raw pointers? :) — Xeo, Mar 01 '12 at 14:19
are you sure that `AudioResource*` is valid? Maybe the iterator is correct, is the backing list that's wrong. — vulkanino, Mar 01 '12 at 14:20
Looks okay as is (nit: post-increment might give worse performance). How do you initialize m_resources? — Sebastian Mach, Mar 01 '12 at 14:20
Everything looks ok. I'd say some other code deleted a pointer from the collection and didn't remove it or set it to `nullptr`. — dario_ramos, Mar 01 '12 at 14:22
Aside from the fact that list is almost certainly the wrong collection, your could should either return an AudioResource* that has getID()==ID or a nullptr. What makes you think what it returns is invalid? — CashCow, Mar 01 '12 at 14:22
@troclosan pre-increment is a minor performance enhancement (which the compiler may optimise away anyway) compared to all the others here. — CashCow, Mar 01 '12 at 14:23
@dario_ramos if that happened you'd get an error in the loop when you try calling ->GetID() on it. Of course it would be undefined behaviour so you could get anything happen including what he OP is seeing, but I doubt that's the bug. — CashCow, Mar 01 '12 at 14:25
@phresnel post-increment leads to creation of a temporary object. and i realize that using of pre-increment not fix subj problem — triclosan, Mar 01 '12 at 14:27
The list is populated because when I went throught with the debugger all the information within the object was still there such as file name so the problem is how I return the object — Chris Condy, Mar 01 '12 at 14:30
@triclosan Actual performance measurements that I did showed no difference between pre- and post-incrementation, despite everyone's claims to the opposite. Compilers aren't that stupid. (Moving the call to `end()` out of the loop did make a difference. But not enough to be worth the effort generally.) — James Kanze, Mar 01 '12 at 14:30
Tackling the problem from another side: How does the call to `getResource` look like? And how do you know that what you return is invalid? This is getting to the point of psychic debugging without further context. Maybe you can reduce the problem to a small http://sscce.org? — Xeo, Mar 01 '12 at 14:56
Added, its just a cin call to get the IDnum, it is invalid because the filename is not the proper filename its a bunch of numbers for one. Secondly if i run the code in debug I can see the value of the filename correctly found. I have found a possible hint to the solution and will edit my code to show you — Chris Condy, Mar 01 '12 at 14:59
std::cout<<*(*list_it)->getFileName(); This line does return the proper information but no how to use that to return in AudioResource* format =s — Chris Condy, Mar 01 '12 at 15:05
@JamesKanze: I published [my benchmark](http://stackoverflow.com/a/9519095/76722), will you publish yours, too? — Sebastian Mach, Mar 01 '12 at 15:39
@phresnel How much of this can be cleaned up? The comment section is getting a little long. Please update the question and/or relevant answers with the information in the comments and then remove them. — casperOne, Mar 01 '12 at 16:43
@ChrisCondy How much of this can be cleaned up? The comment section is getting a little long. Please update the question and/or relevant answers with the information in the comments and then remove them. — casperOne, Mar 01 '12 at 16:43
@JamesKanze How much of this can be cleaned up? The comment section is getting a little long. Please update the question and/or relevant answers with the information in the comments and then remove them. — casperOne, Mar 01 '12 at 16:44
@Xeo How much of this can be cleaned up? The comment section is getting a little long. Please update the question and/or relevant answers with the information in the comments and then remove them. — casperOne, Mar 01 '12 at 16:44

score 2 · Answer 1 · answered Mar 01 '12 at 14:28

2

If type is neither of the two values an uninitialized pointer is added to m_resources:

AudioResource* temp;
if(type == "AUDIO_TYPE_SAMPLE")
{
    temp = new AudioResource(ID,fileName,scope,RESOURCE_AUDIO,AUDIO_TYPE_SAMPLE);
}
else if(type == "AUDIO_TYPE_STREAM")
{
    temp = new AudioResource(ID,fileName,scope,RESOURCE_AUDIO,AUDIO_TYPE_STREAM);
}
m_resources.push_back(temp);

Initialize temp to NULL and only add to m_resources if temp != NULL.

Also, the function returns the same uninitialized pointer.

answered Mar 01 '12 at 14:28

hmjd

120,187
20
207
252

Code has been amended so that nothing is pushed onto the list if is not in the else statement and problem still is there, thanks for trying tho =) – Chris Condy Mar 01 '12 at 14:33
The uninitialized pointer is still being returned: it is used by the caller? – hmjd Mar 01 '12 at 14:36
No for now the return data is ignored, as I have not finished coding the rest of the resource manager =) – Chris Condy Mar 01 '12 at 14:38
But that isn't his code. He is doing push_back within the ifs. True the function will return an invalid pointer but it doesn't get added to the list as far as I see. – CashCow Mar 01 '12 at 14:41
@ChrisCondy, do elements get removed from `m_resources` anywhere? – hmjd Mar 01 '12 at 14:48
@hmjd: See my answer, he already answered that that isn't the case. – Xeo Mar 01 '12 at 14:49
@Xeo, ok. Nothing removes from `m_resources`. Wonder if anything `delete`s an element of `m_resources` without removing it from `m_resources`? – hmjd Mar 01 '12 at 14:52
@ChrisCondy, does anything `delete` an element from `m_resources`? I do not mean remove an element from `m_resources`. – hmjd Mar 01 '12 at 14:55
Nothing is ever removed from m_resources, I havnt even implemented it. And none of the elements are deleted as I have not written code to do that either =s – Chris Condy Mar 01 '12 at 14:56

Xeo · Answer 2 · 2012-03-01T15:02:29.757

1

You return nullptr in case the ID doesn't exist, but you never check against it at the call site, which will give you a null pointer access if the ID doesn't exist and which will likely create problems.

AudioManager::getInstance()->getResource(IDnum)->getFileName();

Change that to

AudioResource* res = AudioManager::getInstance()->getResource(IDnum);
if(res)
  std::cout << res->getFileName();

edited Mar 01 '12 at 15:02

answered Mar 01 '12 at 14:29

Xeo

129,499
52
291
397

Code has been amended so this problem is avoided and still the orignal problem is there – Chris Condy Mar 01 '12 at 14:36
where does he push it into the list? – CashCow Mar 01 '12 at 14:42
@ChrisCondy: Do you have any code that removes things from the list? Do you have concurrent accesses to the list? – Xeo Mar 01 '12 at 14:42
@CashCow: If you would've read the other comments, you would have noticed that the OP *changed* the code *after* our answers pointed the flaws out. – Xeo Mar 01 '12 at 14:43
Nothing removes from the list and no i am not multithreading =) – Chris Condy Mar 01 '12 at 14:44
@ChrisCondy: Okay, last idea... do you maybe access the list after the `AudioManager` is actually destroyed from somewhere else? A dangling pointer maybe? – Xeo Mar 01 '12 at 14:46
Well the AudioManager is an singleton, so know the audiomanager isnt destroyed and the std::list m_resources; so its put on the stack? So maybe I should put this on the heap instead? – Chris Condy Mar 01 '12 at 14:50
Well i tested by making std::list m_resources; a pointer to a list and putting it on the heap and still the same problem – Chris Condy Mar 01 '12 at 14:54
@Chris: Okay, question: How exactly do you know that the return is invalid? What makes you think that? – Xeo Mar 01 '12 at 15:12
Ok for one if i put in the code std::cout<<*(*list_it)->getFileName(); This line does return the proper information. Inside the getresource code =) once it has decided it has found the proper value. – Chris Condy Mar 01 '12 at 15:14
@Xeo he made several revisions, only revision 3 has the faulty code and that wasn't on here for long. If he then changes it to fix the bug inline it becomes no longer a valid question so he should say it has been fixed. – CashCow Mar 01 '12 at 16:00

std::list iterator

2 Answers2