[RESOLVED] Pointer of Iterated Object

[NTL]

I am attempting to write a method which iterates over a list, while looking for a particular element of the list and then returns a pointer to that element. My code is pretty ugly, but I don't understand why it doesn't work. The pointer seems to go out of scope right after the calling method.

Structures::Attribute* getAttribute(Structures::EntityWithAttributes* entityToSearch, std::string attributeToFind){
			
     std::list<Structures::Attribute>::iterator attrIter;
     for(attrIter = entityToSearch->attributes.begin(); attrIter != entityToSearch->attributes.end(); attrIter++){
          if((*attrIter).name == attributeToFind){
               return &(*attrIter);
          }
     }
     // This method is only called when we know the attribute is present, so this shouldn't be reached
     return &Structures::Attribute();
}

Any suggestions would be appreciated.

[OReubens]

don't return pointers or references to variables with local scope (= variables defined inside a function).

[Lindley]

I would point out that the std::find() algorithm with an appropriate functor or lambda would serve this purpose.

[NTL]

don't return pointers or references to variables with local scope (= variables defined inside a function).

In that case, how could I accomplish my goal here?

[Paul McKenzie]

In that case, how could I accomplish my goal here?

By using the std::find_if algorithm function.

#include <algorithm>
//...
struct AttributeFinder
{
   AttributeFinder(const std::string& theName) : m_name(theName) { }

   bool operator() (const Structures::Attribute& attr) const
   { return attr.name == m_name; }

   private:
        std::string m_name;
};

//...
std::list<Structures::Attribute>::iterator attrIter = 
  std::find_if( entityToSearch->attributes.begin(),
                     entityToSearch->attributes.end(), AttributeFinder(attributeToFind));

if ( attrIter != entityToSearch->attributes.end())
   return &(*attrIter);  // found the attribute
else
   return NULL;

In addition, pass strings by reference, not by value.

Regards,

Paul McKenzie

[NTL]

By using the std::find_if algorithm function.

#include <algorithm>
//...
struct AttributeFinder
{
   AttributeFinder(const std::string& theName) : m_name(theName) { }

   bool operator() (const Structures::Attribute& attr) const
   { return attr.name == m_name; }

   private:
        std::string m_name;
};

//...
std::list<Structures::Attribute>::iterator attrIter = 
  std::find_if( entityToSearch->attributes.begin(),
                     entityToSearch->attributes.end(), AttributeFinder(attributeToFind));

if ( attrIter != entityToSearch->attributes.end())
   return &(*attrIter);  // found the attribute
else
   return NULL;

In addition, pass strings by reference, not by value.

Regards,

Paul McKenzie

Why should I pass the string by reference? The purpose of this method is to match the appropriate attribute (from the existing EntityWithAttributes struct) with a string value which is being read in from a file. Therefore, passing the reference to the string would only be passing a reference to a temporary object.

[Lindley]

Why should I pass the string by reference? The purpose of this method is to match the appropriate attribute (from the existing EntityWithAttributes struct) with a string value which is being read in from a file. Therefore, passing the reference to the string would only be passing a reference to a temporary object.

If it's a const reference it's allowed to refer to a temporary. (Unrelated, but FYI: A non-const rvalue reference can also bind to a temporary.) Also, in the event you chose to pass a preexisting string object, it would be slightly more efficient.

It's just the "prefer to pass by const reference rather than by value for non-primitive types" rule of thumb.

[Paul McKenzie]

Why should I pass the string by reference? The purpose of this method is to match the appropriate attribute (from the existing EntityWithAttributes struct) with a string value which is being read in from a file. Therefore, passing the reference to the string would only be passing a reference to a temporary object.

I am referring to your paramter list:'

Structures::Attribute* getAttribute(Structures::EntityWithAttributes* entityToSearch, const std::string& attributeToFind

See the item in red?

You are not changing the string passed to you, so the parameter should be a const reference. Passing by value incurs an unnecessary copy operation -- what if the string were 1,000 characters long? Passing by value would make a copy of this string.

Rarely do you ever pass objects by value -- the exception to this are functors.

Regards,

Paul McKenzie

[NTL]

I believe I understand where you are coming from now, however after making the suggested changes it is still not working properly. Here is an expanded version of the example I provided:

// attribute structure
struct Attribute
{
     std::string name;
     double importance;
};
     

// ... other code
double* importancePtr = &(getAttribute(entityPtr, attributeName)->importance);


Structures::Attribute* getAttribute(Structures::EntityWithAttributes* entityToSearch, std::string attributeToFind){
    
     std::list<Structures::Attribute>::iterator attrIter = 
          std::find_if( entityToSearch->attributes.begin(),
                     entityToSearch->attributes.end(), AttributeFinder(attributeToFind));

     if ( attrIter != entityToSearch->attributes.end())
         return &(*attrIter);  // found the attribute
     else
         return NULL;
}

I would like to note that the above "entityPtr" is retrieved in the same way "getAttribute" works now.

Is there some issue with the way I create the pointer from the structures "importance" value?

[Lindley]

Looks okay to me. What behavior is causing you to think something's wrong?

[NTL]

Looks okay to me. What behavior is causing you to think something's wrong?

Later on in the program when I try and access the value which is pointed to, it shows up as something like 2.17e-311.

[Lindley]

Is it possible you're deleting that object between this point in the code and when you try to access it?

Since the container is a std::list, accidentally invalidating the pointer due to a container reallocation won't happen, so it would probably need to be an actual erase() call. Either that or the pointer itself is getting corrupted.

Unless---hmmm....make sure that the container you're searching isn't a temporary. If the container itself goes poof, obviously anything pointing to it becomes bad.

[NTL]

Is it possible you're deleting that object between this point in the code and when you try to access it?

Since the container is a std::list, accidentally invalidating the pointer due to a container reallocation won't happen, so it would probably need to be an actual erase() call. Either that or the pointer itself is getting corrupted.

Unless---hmmm....make sure that the container you're searching isn't a temporary. If the container itself goes poof, obviously anything pointing to it becomes bad.

After going through my code it doesn't seem like the object being pointed to is deleted before I attempt to access it. Also, I have (I believe correctly) used pointers throughout to reference the objects so I don't have a problem with temporary objects disappearing.

Is there any easy/good way to go through and verify my beliefs of these statements?

[Paul McKenzie]

After going through my code it doesn't seem like the object being pointed to is deleted before I attempt to access it. Also, I have (I believe correctly) used pointers throughout to reference the objects so I don't have a problem with temporary objects disappearing.

Your program crashes, does it not? So how can "good" code crash? You're doing something wrong, and no one knows what it is unless we see the code.

Is there any easy/good way to go through and verify my beliefs of these statements?

What you want is called a "code review", and it takes someone else with experience to look at your code.

Regards,

Paul McKenzie

[NTL]

Your program crashes, does it not? So how can "good" code crash? You're doing something wrong, and no one knows what it is unless we see the code.
What you want is called a "code review", and it takes someone else with experience to look at your code.

Regards,

Paul McKenzie

The code doesn't crash. The only reason I know something is up is because I print out the value which is pointed to when I attempt to use it, however the code still executes all the way through.

Is it possible the problem is something other than the pointer losing its value?