Solved

Invalid pointer hunt

Posted on 2004-09-29
5
406 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
[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
  • 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

What does it mean to be "Always On"?

Is your cloud always on? With an Always On cloud you won't have to worry about downtime for maintenance or software application code updates, ensuring that your bottom line isn't affected.

Question has a verified solution.

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

When writing generic code, using template meta-programming techniques, it is sometimes useful to know if a type is convertible to another type. A good example of when this might be is if you are writing diagnostic instrumentation for code to generat…
C++ Properties One feature missing from standard C++ that you will find in many other Object Oriented Programming languages is something called a Property (http://www.experts-exchange.com/Programming/Languages/CPP/A_3912-Object-Properties-in-C.ht…
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 goal of the video will be to teach the user the difference and consequence of passing data by value vs passing data by reference in C++. An example of passing data by value as well as an example of passing data by reference will be be given. Bot…

631 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