Regression with -Woverloaded-virtual in unit_test_log_t

[Ben Wiederhake <Ben.Wiederhake@…>]

While trying to compile my code with "-Woverloaded-virtual -Werror", I found a shortcoming, possibly even a bug.

In 1.59.0 (and, as far as I can see in git, all following versions), there are the following declarations/definitions:

class test_observer /* boost/test/tree/observer.hpp:40 */
virtual void test_unit_skipped( test_unit const& tu, const_string)
   { test_unit_skipped( tu ); }
virtual void test_unit_skipped( test_unit const& )
   {} ///< backward compartibility

class unit_test_log_t /* boost/test/unit_test_log.hpp:97 */
virtual void test_unit_skipped( test_unit const&, const_string );
/* Actually uses the message in the implementation */
/* Does not override Test_unit_skipped( test_unit const& ) */

The problem with this code is that code like the following simply will not work:

unit_test_log_t& foobar;
test_unit const& tu;
foobar.test_unit_skipped( tu ); // <-- Error!

g++ would say "candidate expects 2 arguments, 1 provided". clang++ is a tad more helpful, and would say: "too few arguments to function call, expected 2, have 1; did you mean 'test_observer::test_unit_skipped'?" In case you're unfamiliar with that behaviour, see this specific answer: ​http://stackoverflow.com/a/6035884/3070326 (Error messages are from a different compilation, but should be the same.)

I hope I have now established that -Woverloaded-virtual "has a point".

I don't fully understand why unit_test_log_t doesn't override the other signature, too; but it should be easily possible to override it with something like:

test_unit_skipped( tu, "<No message>" );

However, I do not yet fully understand the context of unit_test_log_t, so maybe such a call cannot occur. That's why I'm unable to make a patch from that idea.

At the very least, this issue is annoying for people who use Boost.Test and try to compile with -Woverloaded-virtual.

Note that Boost.Test 1.58.0 did *not* exhibit this issue, so I felt free to put "regression" in the subject and 'severity'.

(PS: Thanks to Rene Rivera for redirecting me away from the mailing list, and Raffi Enficiaud for redirecting me to this trac.)

[Ben Wiederhake <Ben.Wiederhake@…>]

Verbatim compiler error message

[Ben Wiederhake <Ben.Wiederhake@…>]

This seems to be the relevant part of the inheritance hierarchy:

test_observer (observer.hpp)
:// No superclasses
virtual void test_unit_skipped( test_unit const& tu, const_string ) { test_unit_skipped( tu ); }
virtual void test_unit_skipped( test_unit const& ) {} ///< backward compartibility

unit_test_log_t (unit_test_log.hpp)
: test_observer
virtual void        test_unit_skipped( test_unit const&, const_string );
// Actual implementation, forwards 'reason' argument as-is to a log_formatter

progress_monitor_t (progress_monitor.hpp)
: test_observer
virtual void    test_unit_skipped( test_unit const&, const_string );
// Actual implementation, ignores 'reason' argument

results_collector_t (results_collector.hpp)	
: test_observer
virtual void        test_unit_skipped( test_unit const&, const_string );
// Actual implementation, ignores 'reason' argument

// No further implementations of 'test_observer'

So I suggest to do the following things:

• The signatures of progress_monitor_t and results_collector_t remove the reason argument in their respective method's signature.
• The classes progress_monitor_t and results_collector_t are completed by a method virtual void test_unit_skipped( test_unit const& tu, const_string ) { test_unit_skipped( tu ); } each.
• The class unit_test_log_t is completed by a method virtual void test_unit_skipped( test_unit const& ) { test_unit_skipped( tu, const_string("<Unknown reason>") ); } or something like that.

Plus, of course, a few tests to make sure that now the signatures test_unit_skipped(test_unit const&) and test_unit_skipped(test_unit const&, const_string) are available to all instances.

I'd love to hear why this is a good / bad idea.

[Ben Wiederhake <Ben.Wiederhake@…>]

Here's a pull request in order to provide grounds for discussion: ​https://github.com/boostorg/test/pull/85

[Gennadiy Rozental]

This warning makes no sense in this case. The code is doing the right thing.

foobar.test_unit_skipped( tu ); is not supposed to work. New Boost.Test code always passes reason why test unit is skipped. The other function only exists to work around the case when someone else outside of Boost.Test implemented test observer based on old semantic. In this case they do not provide 2 arg version and we redirect to one arg version.

I'll incorporate you typo fixes.

Also compile only tests implemented differently in boost.