From edad097a5d347d46d4d1633e0a5f32c4f2b92909 Mon Sep 17 00:00:00 2001 From: Lorenzo Caminiti Date: Fri, 23 Oct 2015 19:38:09 -0700 Subject: [PATCH] tested old-of under a few corner cases and renamed oldof.hpp to old.hpp --- include/boost/contract.hpp | 2 +- include/boost/contract/core/virtual.hpp | 2 +- include/boost/contract/{oldof.hpp => old.hpp} | 169 +++++++++--------- test/Jamfile.v2 | 6 + test/constructor/bases.cpp | 14 +- test/destructor/bases.cpp | 8 +- test/disable/checking.cpp | 5 +- test/function/pre_post.cpp | 7 +- test/oldof/auto.cpp | 27 +++ test/oldof/no_macros.cpp | 155 ++++++++++++++++ test/oldof/no_make_old-error.cpp | 12 ++ test/public_function/bases.hpp | 15 +- 12 files changed, 308 insertions(+), 114 deletions(-) rename include/boost/contract/{oldof.hpp => old.hpp} (52%) create mode 100644 test/oldof/auto.cpp create mode 100644 test/oldof/no_macros.cpp create mode 100644 test/oldof/no_make_old-error.cpp diff --git a/include/boost/contract.hpp b/include/boost/contract.hpp index 7bec56d..1a22245 100644 --- a/include/boost/contract.hpp +++ b/include/boost/contract.hpp @@ -11,7 +11,7 @@ #include #include #include -#include +#include #include #include diff --git a/include/boost/contract/core/virtual.hpp b/include/boost/contract/core/virtual.hpp index 00f54a7..270e937 100644 --- a/include/boost/contract/core/virtual.hpp +++ b/include/boost/contract/core/virtual.hpp @@ -46,7 +46,7 @@ private: // Friendship used to limit library's public API. friend bool copy_old(virtual_*); - friend class old; + friend class convertible_old; template friend class boost::contract::aux::check_subcontracted_pre_post_inv; diff --git a/include/boost/contract/oldof.hpp b/include/boost/contract/old.hpp similarity index 52% rename from include/boost/contract/oldof.hpp rename to include/boost/contract/old.hpp index 2c04598..e294de8 100644 --- a/include/boost/contract/oldof.hpp +++ b/include/boost/contract/old.hpp @@ -1,6 +1,6 @@ -#ifndef BOOST_CONTRACT_OLDOF_HPP_ -#define BOOST_CONTRACT_OLDOF_HPP_ +#ifndef BOOST_CONTRACT_OLD_HPP_ +#define BOOST_CONTRACT_OLD_HPP_ /** @file */ @@ -23,15 +23,14 @@ BOOST_CONTRACT_ERROR_macro_OLDOF_requires_variadic_macros_otherwise_manually_pro /** @cond */ #include +#include #include #include #include /** @endcond */ -// TODO: Consider providing an .old(...) setter for all contracts that will be -// called to copy old-values before body execution but after pre/inv, and will -// call postcondition failure handler in case oldof calc throws (instead of -// making function throw). +// TODO: Can I use unique_ptr instead of shared ptr? If not here... maybe for +// holding contracts, etc. /* PUBLIC */ @@ -48,14 +47,14 @@ BOOST_CONTRACT_ERROR_macro_OLDOF_has_invalid_number_of_arguments_, \ #define BOOST_CONTRACT_ERROR_macro_OLDOF_has_invalid_number_of_arguments_1( \ value) \ - BOOST_CONTRACT_OLDOF_AUTO_TYPEOF_(value)(boost::contract::old( \ - boost::contract::copy_old() ? (value) : boost::contract::old() \ + BOOST_CONTRACT_OLDOF_AUTO_TYPEOF_(value)(boost::contract::make_old( \ + boost::contract::copy_old() ? (value) : boost::contract::make_old() \ )) #define BOOST_CONTRACT_ERROR_macro_OLDOF_has_invalid_number_of_arguments_2( \ v, value) \ - BOOST_CONTRACT_OLDOF_AUTO_TYPEOF_(value)(boost::contract::old(v, \ - boost::contract::copy_old(v) ? (value) : boost::contract::old() \ + BOOST_CONTRACT_OLDOF_AUTO_TYPEOF_(value)(boost::contract::make_old(v, \ + boost::contract::copy_old(v) ? (value) : boost::contract::make_old() \ )) #ifdef BOOST_NO_CXX11_AUTO_DECLARATIONS @@ -66,7 +65,7 @@ BOOST_CONTRACT_ERROR_macro_OLDOF_has_invalid_number_of_arguments_, \ /** @endcond */ // Explicitly force shared_ptr conversion to allow for C++11 auto decl. # define BOOST_CONTRACT_OLDOF_AUTO_TYPEOF_(value) \ - boost::shared_ptr + boost::contract::old_ptr #endif #endif // VARIADICS @@ -75,6 +74,31 @@ BOOST_CONTRACT_ERROR_macro_OLDOF_has_invalid_number_of_arguments_, \ namespace boost { namespace contract { +// Used to avoid exposing entire shared_ptr API (e.g., client stealing +// ownership of this pointer could break this lib). +template +class old_ptr // Copyable (as *). + : boost::spirit::classic::safe_bool > +{ +public: + explicit old_ptr() {} + + // operator safe-bool() const (see below). + + T const& operator*() const { return ptr_.operator*(); } + T const* operator->() const { return ptr_.operator->(); } + +private: + explicit old_ptr(boost::shared_ptr ptr) : ptr_(ptr) {} + + bool operator_bool() const { return ptr_.operator bool(); } + + boost::shared_ptr ptr_; + + friend struct boost::spirit::classic::safe_bool; + friend class convertible_old; +}; + bool copy_old() { #if BOOST_CONTRACT_CONFIG_NO_POSTCONDITIONS return false; // Post checking disabled, so never copy old values. @@ -93,90 +117,47 @@ bool copy_old(virtual_* v) { #endif } -// TODO: Rename this make_old (that is is different from .old(...)) and add -// old_ptr so with and without macros: -// auto x = BOOST_CONTRACT_OLDOF(v, x); -// old_ptr x = BOOST_CONTRACT_OLDOF(v, x); -// old_ptr x = make_old(v, copy_old(v) ? x : make_old()); -// plus, make_old() return old_erasure type without operator old_ptr so -// following correctly gives compiler error: -// old_ptr = copy_old(v) ? x : make_old(); -// -// template -// class old_ptr { -// public: -// T const& operator*() const; -// T const* operator->() const; -// -// private: -// boost::shared_ptr ptr_; -// }; -// -// class old_erasure { -// public: -// // Implicitly called by using ternary operator `... ? ... : make_old()`. -// template -// /* implicit */ old_erasure(T const& old); -// -// friend old_erasure make_old() { return old_erasure(); } -// -// private: -// explicit old_erasure(); -// }; -// -// class old_unerasure { -// public: -// // Implicitly called by constructor init `old_ptr old_x = ...`. -// template -// operator old_ptr(); -// -// friend old_unerasure make_old(old_erasure const& old) { -// return old_unerasure(0, old); -// } -// -// friend old_unerasure make_old(virtual_* v, old_erasure const& old) { -// return old_unerasure(v, old); -// } -// -// private: -// explicit old_unerasure(virtual_* v, old_erasure const& old); -// }; -class old { // Copyable (as *). +class unconvertible_old { // Copyable (as *). public: - explicit old() : v_(0), value_() {} - - explicit old(virtual_* v, old const& other) : v_(v), value_(other.value_) {} - + // Implicitly called by ternary operator `... ? ... : make_old()`. template - /* implicit */ old(T const& old_value) : v_(0), - value_(boost::make_shared(old_value)) {} // T's one single copy. + /* implicit */ unconvertible_old(T const& old_value) : + ptr_(boost::make_shared(old_value)) {} // T's one single copy. - // TODO: I might be able to use unique_ptr here instead of shared_ptr. That - // might be true for the pointer that holds contract and call as well... - // do some testing to figure that out (unique_ptr adds less overhead). +private: + explicit unconvertible_old() {} + boost::shared_ptr ptr_; + + friend class convertible_old; + friend unconvertible_old make_old(); +}; + +class convertible_old { // Copyable (as *). +public: + // Implicitly called by constructor init `old_ptr old_x = ...`. template - operator boost::shared_ptr() { + /* implicit */ operator old_ptr() { if(!v_ && boost::contract::aux::check_guard::checking()) { // Return null shared ptr (see after if statement). } else if(!v_) { - BOOST_CONTRACT_AUX_DEBUG(value_); - boost::shared_ptr old_value = - boost::static_pointer_cast(value_); - BOOST_CONTRACT_AUX_DEBUG(old_value); - return old_value; + BOOST_CONTRACT_AUX_DEBUG(ptr_); + boost::shared_ptr old = + boost::static_pointer_cast(ptr_); + BOOST_CONTRACT_AUX_DEBUG(old); + return old_ptr(old); } else if(v_->action_ == boost::contract::virtual_::push_old_init || v_->action_ == boost::contract::virtual_::push_old_copy) { - BOOST_CONTRACT_AUX_DEBUG(value_); + BOOST_CONTRACT_AUX_DEBUG(ptr_); if(v_->action_ == boost::contract::virtual_::push_old_init) { - v_->old_inits_.push(value_); + v_->old_inits_.push(ptr_); } else { - v_->old_copies_.push(value_); + v_->old_copies_.push(ptr_); } - return boost::shared_ptr(); + return old_ptr(); } else if(v_->action_ == boost::contract::virtual_::pop_old_init || v_->action_ == boost::contract::virtual_::pop_old_copy) { - BOOST_CONTRACT_AUX_DEBUG(!value_); + BOOST_CONTRACT_AUX_DEBUG(!ptr_); boost::shared_ptr value; if(v_->action_ == boost::contract::virtual_::pop_old_init) { value = v_->old_inits_.front(); @@ -189,19 +170,35 @@ public: } else { v_->old_copies_.pop(); } - boost::shared_ptr old_value = + boost::shared_ptr old = boost::static_pointer_cast(value); - BOOST_CONTRACT_AUX_DEBUG(old_value); - return old_value; + BOOST_CONTRACT_AUX_DEBUG(old); + return old_ptr(old); } - BOOST_CONTRACT_AUX_DEBUG(!value_); - return boost::shared_ptr(); + BOOST_CONTRACT_AUX_DEBUG(!ptr_); + return old_ptr(); } private: + explicit convertible_old(virtual_* v, unconvertible_old const& old) : + v_(v), ptr_(old.ptr_) {} + virtual_* v_; - boost::shared_ptr value_; + boost::shared_ptr ptr_; + + friend convertible_old make_old(unconvertible_old const&); + friend convertible_old make_old(virtual_*, unconvertible_old const&); }; + +unconvertible_old make_old() { return unconvertible_old(); } + +convertible_old make_old(unconvertible_old const& old) { + return convertible_old(0, old); +} + +convertible_old make_old(virtual_* v, unconvertible_old const& old) { + return convertible_old(v, old); +} } } // namespace diff --git a/test/Jamfile.v2 b/test/Jamfile.v2 index f44c18b..1cd9b44 100644 --- a/test/Jamfile.v2 +++ b/test/Jamfile.v2 @@ -25,6 +25,12 @@ test-suite function : [ subdir-run function : body_throw ] ; +test-suite oldof : + [ subdir-run oldof : no_macros ] + [ subdir-run oldof : auto ] + [ subdir-compile-fail oldof : no_make_old-error ] +; + test-suite disable : [ subdir-run disable : checking ] ; diff --git a/test/constructor/bases.cpp b/test/constructor/bases.cpp index 810dd94..0656806 100644 --- a/test/constructor/bases.cpp +++ b/test/constructor/bases.cpp @@ -6,7 +6,7 @@ #include #include #include -#include +#include #include #include #include @@ -45,8 +45,8 @@ struct t BOOST_CONTRACT_ASSERT(z.value < 0); }) { - boost::shared_ptr old_z; - boost::shared_ptr old_l = + boost::contract::old_ptr old_z; + boost::contract::old_ptr old_l = BOOST_CONTRACT_OLDOF(l_type::eval(l)); boost::contract::guard c = boost::contract::constructor(this) .old([&] { @@ -108,9 +108,9 @@ struct c }), t<'d'>(dz), t<'p'>(pz), t<'q'>(qz), t<'e'>(ez) { - boost::shared_ptr old_y = + boost::contract::old_ptr old_y = BOOST_CONTRACT_OLDOF(y_type::eval(y)); - boost::shared_ptr old_m; + boost::contract::old_ptr old_m; boost::contract::guard c = boost::contract::constructor(this) .old([&] { out << "c::ctor::old" << std::endl; @@ -183,8 +183,8 @@ struct a }), b(), c(y, dz, pz, qz, ez) { - boost::shared_ptr old_x; - boost::shared_ptr old_n = + boost::contract::old_ptr old_x; + boost::contract::old_ptr old_n = BOOST_CONTRACT_OLDOF(n_type::eval(n)); boost::contract::guard c = boost::contract::constructor(this) .old([&] { diff --git a/test/destructor/bases.cpp b/test/destructor/bases.cpp index cebaf8f..a93dae0 100644 --- a/test/destructor/bases.cpp +++ b/test/destructor/bases.cpp @@ -6,7 +6,7 @@ #include #include #include -#include +#include #include #include #include @@ -31,7 +31,7 @@ struct t { explicit t() : k_(-1) { ++l.value; } virtual ~t() { - boost::shared_ptr old_l; + boost::contract::old_ptr old_l; boost::contract::guard c = boost::contract::destructor(this) .old([&] { out << Id << "::dtor::old" << std::endl; @@ -78,7 +78,7 @@ struct c explicit c() : j_(-1) { ++m.value; } virtual ~c() { - boost::shared_ptr old_m = + boost::contract::old_ptr old_m = BOOST_CONTRACT_OLDOF(m_type::eval(m)); boost::contract::guard c = boost::contract::destructor(this) .old([&] { @@ -132,7 +132,7 @@ struct a explicit a() : i_(-1) { ++n.value; } virtual ~a() { - boost::shared_ptr old_n; + boost::contract::old_ptr old_n; boost::contract::guard c = boost::contract::destructor(this) .old([&] { out << "a::dtor::old" << std::endl; diff --git a/test/disable/checking.cpp b/test/disable/checking.cpp index 2508090..d58f9d6 100644 --- a/test/disable/checking.cpp +++ b/test/disable/checking.cpp @@ -3,11 +3,10 @@ #include "../aux_/oteststream.hpp" #include "../aux_/counter.hpp" -#include +#include #include #include #include -#include #include #include @@ -22,7 +21,7 @@ struct a { int f(x_type& x) { int result; - boost::shared_ptr old_x = BOOST_CONTRACT_OLDOF( + boost::contract::old_ptr old_x = BOOST_CONTRACT_OLDOF( x_type::eval(x)); boost::contract::guard c = boost::contract::public_function(this) .precondition([&] { out << "a::f::pre" << std::endl; }) diff --git a/test/function/pre_post.cpp b/test/function/pre_post.cpp index c61c471..8f00bbd 100644 --- a/test/function/pre_post.cpp +++ b/test/function/pre_post.cpp @@ -4,10 +4,9 @@ #include "../aux_/oteststream.hpp" #include "../aux_/counter.hpp" #include -#include +#include #include #include -#include #include #include @@ -18,9 +17,9 @@ struct y_tag; typedef boost::contract::aux::test::counter y_type; bool swap(x_type& x, y_type& y) { bool result; - boost::shared_ptr old_x = + boost::contract::old_ptr old_x = BOOST_CONTRACT_OLDOF(x_type::eval(x)); - boost::shared_ptr old_y; + boost::contract::old_ptr old_y; boost::contract::guard c = boost::contract::function() .precondition([&] { out << "swap::pre" << std::endl; diff --git a/test/oldof/auto.cpp b/test/oldof/auto.cpp new file mode 100644 index 0000000..5f7de53 --- /dev/null +++ b/test/oldof/auto.cpp @@ -0,0 +1,27 @@ + +// Test that OLDOF macro allows to use C++11 auto declarations. + +#include +#include +#include +#include +#include + +int main() { +#ifdef BOOST_NO_CXX11_AUTO_DECLARATIONS + std::cout << "No C++11 auto declarations (nothing to test)" << std::endl; +#else + int x = 0; + auto old_x = BOOST_CONTRACT_OLDOF(x); + BOOST_STATIC_ASSERT(boost::is_same >::value); + + boost::contract::virtual_* v = 0; + char y = 'a'; + auto old_y = BOOST_CONTRACT_OLDOF(v, y); + BOOST_STATIC_ASSERT(boost::is_same >::value); +#endif + return 0; +} + diff --git a/test/oldof/no_macros.cpp b/test/oldof/no_macros.cpp new file mode 100644 index 0000000..23f7a15 --- /dev/null +++ b/test/oldof/no_macros.cpp @@ -0,0 +1,155 @@ + +// Test old values without BOOST_CONTRACT_OLDOF macro. + +#include "../aux_/oteststream.hpp" +#include "../aux_/counter.hpp" +#include +#include +#include +#include +#include +#include +#include +#include +#include + +boost::contract::aux::test::oteststream out; + +struct i_tag; typedef boost::contract::aux::test::counter i_type; +struct j_tag; typedef boost::contract::aux::test::counter j_type; + +struct b { + virtual void swap(i_type& i, j_type& j, boost::contract::virtual_* v = 0); +}; + +void b::swap(i_type& i, j_type& j, boost::contract::virtual_* v) { + boost::contract::old_ptr old_i = boost::contract::make_old(v, + boost::contract::copy_old(v) ? + i_type::eval(i) + : + boost::contract::make_old() + ); + boost::contract::old_ptr old_j; + boost::contract::guard c = boost::contract::public_function(v, this) + .precondition([&] { + out << "b::swap::pre" << std::endl; + BOOST_CONTRACT_ASSERT(i.value != j.value); + }) + .old([&] { + out << "b::swap::old" << std::endl; + old_j = boost::contract::make_old(v, boost::contract::copy_old(v) ? + j_type::eval(j) : boost::contract::make_old()); + }) + .postcondition([&] { + out << "b::swap::post" << std::endl; + BOOST_CONTRACT_ASSERT(i.value == old_j->value); + BOOST_CONTRACT_ASSERT(j.value == old_i->value); + }) + ; + assert(false); +}; + +struct a + #define BASES public b + : BASES +{ + typedef BOOST_CONTRACT_BASE_TYPES(BASES) base_types; + #undef BASES + + void swap(i_type& i, j_type& j, boost::contract::virtual_* v = 0) + /* override */ { + boost::contract::guard c = boost::contract::public_function< + override_swap>(v, &a::swap, this, i, j); + + out << "a::swap::body" << std::endl; + int t = i.value; + i.value = j.value; + j.value = t; + } + BOOST_CONTRACT_OVERRIDE(swap) +}; + +struct x_tag; typedef boost::contract::aux::test::counter x_type; +struct y_tag; typedef boost::contract::aux::test::counter y_type; + +void swap(x_type& x, y_type& y) { + boost::contract::old_ptr old_x = boost::contract::make_old( + boost::contract::copy_old() ? + x_type::eval(x) + : + boost::contract::make_old() + ); + boost::contract::old_ptr old_y; + boost::contract::guard c = boost::contract::function() + .precondition([&] { + out << "swap::pre" << std::endl; + BOOST_CONTRACT_ASSERT(x.value != y.value); + }) + .old([&] { + out << "swap::old" << std::endl; + old_y = boost::contract::make_old(boost::contract::copy_old() ? + y_type::eval(y) : boost::contract::make_old()); + }) + .postcondition([&] { + out << "swap::post" << std::endl; + BOOST_CONTRACT_ASSERT(x.value == old_y->value); + BOOST_CONTRACT_ASSERT(y.value == old_x->value); + }) + ; + + out << "swap::body" << std::endl; + char t = x.value; + x.value = y.value; + y.value = t; +} + +int main() { + std::ostringstream ok; + + out.str(""); + x_type x; x.value = 'a'; + y_type y; y.value = 'b'; + swap(x, y); + + ok.str(""); ok + << "swap::pre" << std::endl + << "swap::old" << std::endl + << "swap::body" << std::endl + << "swap::post" << std::endl + ; + BOOST_TEST(out.eq(ok.str())); + + BOOST_TEST_EQ(x.value, 'b'); + BOOST_TEST_EQ(x.copies(), 1); + BOOST_TEST_EQ(x.evals(), 1); + + BOOST_TEST_EQ(y.value, 'a'); + BOOST_TEST_EQ(y.copies(), 1); + BOOST_TEST_EQ(y.evals(), 1); + + a aa; + i_type i; i.value = 1; + j_type j; j.value = 2; + out.str(""); + aa.swap(i, j); + + ok.str(""); ok + << "b::swap::pre" << std::endl + << "b::swap::old" << std::endl + << "a::swap::body" << std::endl + << "b::swap::old" << std::endl + << "b::swap::post" << std::endl + ; + BOOST_TEST(out.eq(ok.str())); + + BOOST_TEST_EQ(i.value, 2); + BOOST_TEST_EQ(i.copies(), 1); + BOOST_TEST_EQ(i.evals(), 1); + + BOOST_TEST_EQ(j.value, 1); + BOOST_TEST_EQ(j.copies(), 1); + BOOST_TEST_EQ(j.evals(), 1); + + return boost::report_errors(); +} + diff --git a/test/oldof/no_make_old-error.cpp b/test/oldof/no_make_old-error.cpp new file mode 100644 index 0000000..b754bf1 --- /dev/null +++ b/test/oldof/no_make_old-error.cpp @@ -0,0 +1,12 @@ + +// Test error when make_old(...) not used by mistake. + +#include + +int main() { + int x = 1; + boost::contract::old_ptr old_x = boost::contract::copy_old() ? x : + boost::contract::make_old(); // Error (missing outer make_old(...)). + return 0; +} + diff --git a/test/public_function/bases.hpp b/test/public_function/bases.hpp index 3f17606..2fdaea1 100644 --- a/test/public_function/bases.hpp +++ b/test/public_function/bases.hpp @@ -9,10 +9,9 @@ #include #include #include -#include +#include #include #include -#include #include boost::contract::aux::test::oteststream out; @@ -52,9 +51,9 @@ template result_type& t::f(s_type& s, boost::contract::virtual_* v) { std::ostringstream r; r << "none-" << Id; static result_type result(r.str()); - boost::shared_ptr old_z = + boost::contract::old_ptr old_z = BOOST_CONTRACT_OLDOF(v, z_type::eval(z)); - boost::shared_ptr old_s; + boost::contract::old_ptr old_s; boost::contract::guard c = boost::contract::public_function(v, result, this) .precondition([&] { out << Id << "::f::pre" << std::endl; @@ -100,9 +99,9 @@ struct c virtual result_type& f(s_type& s, boost::contract::virtual_* v = 0) /* override */ { static result_type result("none-c"); - boost::shared_ptr old_y = + boost::contract::old_ptr old_y = BOOST_CONTRACT_OLDOF(v, y_type::eval(y)); - boost::shared_ptr old_s; + boost::contract::old_ptr old_s; boost::contract::guard c = boost::contract::public_function< override_f>(v, result, &c::f, this, s) .precondition([&] { @@ -182,9 +181,9 @@ struct a result_type& f(s_type& s, boost::contract::virtual_* v = 0) /* override */ { static result_type result("none-a"); - boost::shared_ptr old_x = + boost::contract::old_ptr old_x = BOOST_CONTRACT_OLDOF(v, x_type::eval(x)); - boost::shared_ptr old_s; + boost::contract::old_ptr old_s; boost::contract::guard c = boost::contract::public_function< override_f>(v, result, &a::f, this, s) .precondition([&] {