Gennadiy Rozental wrote:
Peter Klotz wrote:
Gennadiy Rozental wrote:
Peter Klotz
writes: Why not just adding the virtual destructor to make valgrind happy?
Did you try removing virtual from test_suite destructor instead?
Yes, just making ~test_suite() non virtual worsens things.
valgrind complains about twice as many "invalid read" errors.
[...]
Making ~test_unit() virtual removes all errors. Then even a non virtual ~test_suite() is no longer a problem.
So the final conclusion would be to remove virtual from ~test_suite() and add it to the base class destructor ~test_unit().
Peter,
I am not convinced there is a bug inside the Boost.Test code. As far as I can tell all I am doing is legitimate. Maybe we should report this to the valgrind developers instead?
Hello Gennadiy I finally found the time to extract the code portion from Boost.Test that shows the valgrind error. Meanwhile I have switched to Boost 1.39.0, gcc 4.3.3 and valgrind 3.4.1. The attached sample is a standalone program and can be compiled like this: g++ -g -W -Wall BoostTestValgrindError.cpp -o BoostTestValgrindError Running it with valgrind (valgrind ./BoostTestValgrindError) shows the error. I stripped everything that is not necessary to reproduce the problem. The internal map of test cases and test suites is replaced by a set. The reason for the error seems to be the downcast in your clear() method in framework_impl. Assigning the downcasted pointer to a variable and then performing the delete resolves the problem. There seems to be a subtle difference between these two alternatives. My suggestion that would also clean up the clear() method is the following: * Add the virtual destructor to class test_unit * Rewrite framework_impl::clear() this way: void clear() { while( !m_test_units.empty() ) delete m_test_units.begin()->second; // the delete will erase this element from the map } The virtual destructor ensures that you can safely destroy your derived classes through the base class pointer stored in m_test_units. The distinction between test suites and test cases in clear() is then no longer needed and the downcast is also gone. Regards, Peter.