Solved

Invalid pointer hunt

Posted on 2004-09-29
5
387 Views
Last Modified: 2012-05-05
OK, this might be tricky, but I'll post all the code I think is relevant and hope that someone is willing to take the time and see if they can find what's wrong with my code.

I'm implementing a triangle mesh class (which currently uses the boost graph library for internal storage). My problem is that something seems to be wrong with my code for inserting vertices into the mesh. When loading a mesh from file, it seems to work just fine, but when using the same add_vertex function to copy select vertices from one mesh to another, only garbage gets inserted.

I think it must be a problem with scoping in some way, but I can't quite see what goes wrong.

Are these code snippets enough to see what's going on?

////////////////////////////////////////////////

using namespace std;

int main( )
{
  Mesh<3> m;   // Create two three-dimensional triangel meshes.
  Mesh<3> m2;

   m.load( "data/file1.wrl" );
   //m2.load( "data/file2.wrl" );     // If m2 is intialized like this, all is fine
   m2 = ICP_Tools::random_subset( m, 10 ); // If m2 is initialized like this, weird things happen
 
    IO::output << "\n Vertices:\n" ;
    Mesh<3>::Vertex_Iterator vertex( m.vertices().first );
    Mesh<3>::Vertex_Iterator vertex_end( m.vertices().second );
    for (  ; vertex != vertex_end;  ++vertex )
    {
      IO::output << *vertex << "\n";  // The outputs here look fine
    }

    IO::output << "\n Vertices:\n" ;
    vertex = m2.vertices().first;
    vertex_end = m2.vertices().second ;
    for (  ; vertex != vertex_end;  ++vertex )
    {
      IO::output << *vertex << "\n"; // The outputs here are vertices with random numbers, or a seg fault
    }
   
  return 0;
}

////////////////////////////////////////////////

template< unsigned int TDim >
class Mesh
{
  // snip...

  void load( const string file_name )
  {  
    // Open file, some basic setup, start reading values...

    Vertex<3> mesh_point;
    vector< long double > p;

    while (fscanf( in_file, "%Lf %Lf %Lf,", &p[0], &p[1], &p[2] ))
      {
      nof_points++;
      mesh_point.set_position( p );
      this->add_vertex( mesh_point );
      }

    // Do more reading, close file...
  }

  void add_vertex( Vertex<TDim> v )
  {
    Graph_Vertex vv = boost::add_vertex( its_vertices );
    its_vertex_data[vv] = v; // All of this is pass-by-value, so a copy of the vertex v should be copied here, right?
  }

  // snip...
};

////////////////////////////////////////////////

namespace ICP_Tools
{

Mesh<3> random_subset( Mesh<3> const& mesh, unsigned int size )
{
  Mesh<3> the_subset;

  // Use STL random sample algorithm to pick points:
  Vertex<3> temp_holder[size];
  Mesh<3>::Const_Vertex_Iterator first_vertex = mesh.const_vertices().first;
  Mesh<3>::Const_Vertex_Iterator last_vertex  = mesh.const_vertices().second;
  __gnu_cxx::random_sample_n( first_vertex, last_vertex,
                        temp_holder, size );

  for (unsigned int i = 0; i < size; ++i)
    {
      the_subset.add_vertex( temp_holder[i] );   // Here, vertices are copied to the new Mesh.
    }

  return the_subset;
}

} // end namespace ICP_Tools
0
Comment
Question by:loveslave
  • 3
  • 2
5 Comments
 
LVL 13

Expert Comment

by:SteH
ID: 12178440
the_subset is a local variable which will go out of scope when returning from the function random_subset. You either need to add a reference to m2 as a parameter to the call or return a mesh allocated on the heap:

Mesh<3>* random_subset( Mesh<3> const& mesh, unsigned int size )
{
  Mesh<3>* the_subset = new Mesh<3>[size];

  // Use STL random sample algorithm to pick points:
  Vertex<3> temp_holder[size];
  Mesh<3>::Const_Vertex_Iterator first_vertex = mesh.const_vertices().first;
  Mesh<3>::Const_Vertex_Iterator last_vertex  = mesh.const_vertices().second;
  __gnu_cxx::random_sample_n( first_vertex, last_vertex,
                     temp_holder, size );

  for (unsigned int i = 0; i < size; ++i)
    {
     the_subset->add_vertex( temp_holder[i] );   // Here, vertices are copied to the new Mesh.
    }

  return the_subset;
}
And don't forget to delete [] the returned value when it is no longer used.
} // end namespace ICP_Tools
0
 
LVL 1

Author Comment

by:loveslave
ID: 12178758
Ah, but I thought that the_subset would be copied when returning from the function. Isn't it?

It does work when I supply the_subset as an out argument instead, though.
0
 
LVL 13

Accepted Solution

by:
SteH earned 500 total points
ID: 12178856
The return value will be copied. For a built in type it will the value. But in the case of  a complex variable the adress will be copied. And that address is pointing on the stack which will most likely be overwritten. In that case the address is pointing to garbage as you found out.

If you supply the_subset as output parameter it will work. That case is more or less equal to the case that you add a reference to m2: only keep in mind that you either need a pointer to that variable or a reference so that changes to its structure are reflected back to the calling function:

Mesh<3>* random_subset( Mesh<3> const& mesh, Mesh<3>& out_mesh, const unsigned int size )
          The first parameter could be passed as value as well but it more efficient only coipying the address.
          Second parameter needs to be reference
          Third parameter should be const, shouldn't it?
0
 
LVL 1

Author Comment

by:loveslave
ID: 12187738
I feel a bit worried now... I've been programming for many years, and this hasn't really come up before. Are you saying that you can never safely return anything but a POD-type? What about std::vector and the like?

Would it not be safe to use the return value from the following function?
Vertex<3> translate( const Vertex<3>& v )
{
  Vertex<3> result;
  result = some_operation_on( v );
  return result;
}
0
 
LVL 13

Expert Comment

by:SteH
ID: 12187877
You can return other types, you are right. I think you will need a copy constructor and assignment operator for Mesh<3>. If this is missing a default one is used which copies built-in types and for the rest addresses.
In that case you need to add
Mesh<3> (&Mesh<3>) {...};
Mesh<3>& operator= ... {...};
to your class.


0

Featured Post

Optimizing Cloud Backup for Low Bandwidth

With cloud storage prices going down a growing number of SMBs start to use it for backup storage. Unfortunately, business data volume rarely fits the average Internet speed. This article provides an overview of main Internet speed challenges and reveals backup best practices.

Question has a verified solution.

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

Suggested Solutions

Title # Comments Views Activity
C Language combined operators 28 109
ASP.net build a IF/Then Walkthrough Guide 1 212
I could not build boost code, 10 88
object oriented programming comparison 5 72
Often, when implementing a feature, you won't know how certain events should be handled at the point where they occur and you'd rather defer to the user of your function or class. For example, a XML parser will extract a tag from the source code, wh…
Many modern programming languages support the concept of a property -- a class member that combines characteristics of both a data member and a method.  These are sometimes called "smart fields" because you can add logic that is applied automaticall…
The goal of the video will be to teach the user the concept of local variables and scope. An example of a locally defined variable will be given as well as an explanation of what scope is in C++. The local variable and concept of scope will be relat…
The viewer will be introduced to the member functions push_back and pop_back of the vector class. The video will teach the difference between the two as well as how to use each one along with its functionality.

809 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