Re: [Boost-users] [Boost.Test] valgrind complains about invalid reads

Subject: Re: [Boost-users] [Boost.Test] valgrind complains about invalid reads
From: Scott Banachowski (sbanacho_at_[hidden])
Date: 2009-12-17 16:18:23

Thanks for the reply Gennadiy, see inline for comments:

--- On Wed, 12/16/09, Gennadiy Rozental <rogeeff_at_[hidden]> wrote:

> From: Gennadiy Rozental <rogeeff_at_[hidden]>
> Subject: Re: [Boost-users] [Boost.Test] valgrind complains about invalid    reads
> To: boost-users_at_[hidden]
> Date: Wednesday, December 16, 2009, 9:32 PM
> Scott Banachowski <sbanacho
> <at> yahoo.com> writes:
> 
> > 
> > 
> > I agree with Peter here, the destructor for test_unit
> should be virtual.  
> 
> 'Should' is a strong statement. Do you have anything to
> based it on? 
> 

Ok, should is too strong.  Change that to "it would be nice to", because the possibility that the base class destructor may be called (even if not by your code, possibly by others).

> > I find that it is being deleted when
> > test_unit* is a stored in reference counted pointer, 
> 
> Where?

My mistake, it's held in a map.  See the next comment:

> 
> > and at program exit it is being deleted by calling
> ~test_unit().  
> 
> Where do I call destructor explicitly?

You don't.  It's being called implicitly at program exit, by valgrind.  
I think what is happening is that valgrind replaces the delete operator with its own, which is "operator delete(void*)"  The typeinfo is likely lost by this replacement of delete.

Just because you don't call the destructor, it doesn't mean others won't.  If a base class is derived from, best practice is to declare its destructor virtual.  It is relatively common to replace allocators (e.g. tcmalloc).

> 
> > If you use a derived class (test_suite), this is
> leading to an error.
> 
> Where?

As already mentioned, reported by valgrind. 

> 
> > Also, in the code I'm looking at (1.40.0), the
> test_suite class has a 
> > virtual destructor, but its base class  test_unit
> does not.  
> > Not making the base class virtual seems to be
> defeating the purpose. 
> 
> What purpose? The only reason that test_suite destructor is
> virtual is because
> this class in fact is being polymorphically. It has
> subclass master_test_suite.
> 

By defeating the purpose, I mean it forces code like the following:

if( ut_detail::test_id_2_unit_type( tu.second->p_id ) == tut_suite )
    delete  static_cast<test_suite const*>(tu.second);
else
    delete  static_cast<test_case const*>(tu.second);

A virtual base case simplifies this code (especially if in the future other classes are added, or in the case of replacement allocator it does the right thing).

thanks, 
Scott

> Gennadiy
> 
> _______________________________________________
> Boost-users mailing list
> Boost-users_at_[hidden]
> http://lists.boost.org/mailman/listinfo.cgi/boost-users
>