[multiindex] internal scope_guard has changed access from public to protected
Hi Joaquín, I'm using the scope_guard from multi_index/detail/scope_guard.hpp. It seems like a revision has made the destructor and copy-constructor protected instead of private. This is in some sense more idiomatic, but it breaks a use that I have in my upcomming auto_buffer class. Basically I'm only constructing a scope_guard object when I know the code can throw, otherwise I construct a "null" scope_guard which is simply an instance of scope_guard_impl_base. Here's the code: namespace auto_buffer_detail { typedef boost::multi_index::detail::scope_guard_impl_base null_guard; } #define BOOST_AUTO_BUFFER_CONSTRUCTOR_SCOPE_GUARD() \ boost::has_nothrow_copy<T>::value ? boost::auto_buffer_detail::null_guard() : \ boost::multi_index::detail::make_obj_guard( *this, \ &auto_buffer::deallocate, \ buffer_, members_.capacity_ ) and use it like so: boost::multi_index::detail::scope_guard guard = BOOST_AUTO_BUFFER_CONSTRUCTOR_SCOPE_GUARD(); ... guard.dismiss(); Any chance you make the two members public again? -Thorsten
Thorsten Ottosen wrote:
s/private/public/, I assume
Why not provide a "null" scope_guard as a first class citizen rather than simply exposing implementation details to enable this use case? _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
Stewart, Robert <Robert.Stewart <at> sig.com> writes:
Yep, why can't you just define null guard as struct null_guard:boost::multi_index::detail::scope_guard_impl_base{}; Unrelated to your particular problem, is this auto_buffer class of yours part of Boost or a private project? If the former, we should think about moving scope_guard to boost/detail/ where implementation bits common to several libs inhabit. Joaquín M López Muñoz Telefónica, Investigación y Desarrollo
Joaquin M Lopez Munoz skrev:
Could be, I didn't check.
the ?: operator seem to require that the second argument can be converted to the type of the first argument.
That's a good idea. I'm using it for an upcomming library called Boost.AutoBuffer. I'm confident that other boost libs also use it. -Thorsten
Thorsten Ottosen <nesotto <at> cs.aau.dk> writes:
I might be missing something, but I think tou can simply write the following to circumvent the problem: #define BOOST_AUTO_BUFFER_CONSTRUCTOR_SCOPE_GUARD() \ boost::has_nothrow_copy<T>::value ? \ static_cast<boost::multi_index::detail::scope_guard>( \ boost::auto_buffer_detail::null_guard()) : \ static_cast<boost::multi_index::detail::scope_guard>( \ boost::multi_index::detail::make_obj_guard( \ *this, \ &auto_buffer::deallocate, \ buffer_, members_.capacity_ )) Does this work? Additionally, why are you doing the guard selection on run time? boost::has_nothrow_copy<T>::value is a compile-time value so you can select the exact type of the guard (null or otherwise) with some Boost.MPL.
I've just made a global search and the only other lib using it is Signals2, inside a detail component of yours (the one you intend to promote to official lib status, I presume). I'll add this move to boost/detail/ to my todo list. Joaquín M López Muñoz Telefónica, Investigación y Desarrollo
Hi Joaquin, Sorry for the slow response. Joaquin M Lopez Munoz skrev:
It seems to work.
Well, I'm lazy, so I didn't want to implement the functionality twice with and without the guard. IMO the code is very clear with this macro. I think the optimizer has no problem removing the empty guard. Thanks -Thorsten
Thorsten Ottosen skrev:
I take that back. operator ?: seems to generate a redundant copy which kills this approach.
It seems to me that creating a null-guard conditioned on some compile-time value is quite common in containers that copy elements around. Would it not be possible to add a bool template parameter to make_obj_guard() such we can simply write scope_guard g = make_obj_guard<some_condition<T>::value>(...); ? -Thorsten
Thorsten Ottosen <nesotto <at> cs.aau.dk> writes:
Yep, this seems like a general enough functionality to be included in scope_guard.hpp. I'd prefer to leave make_obj_guard like it is and add an additional make_obj_guard_if<...> (note the _if) for your use case, which creates a null_guard if the condition is not true. Would you like to do the addition yourself and commit the changes to the trunk? I'm quite busy these days to do it myself. Joaquín M López Muñoz Telefónica, Investigación y Desarrollo
Thorsten Ottosen skrev:
Joaquin M Lopez Munoz skrev:
Thorsten Ottosen <nesotto <at> cs.aau.dk> writes:
Attached is a patch. I have used the header in my own code like so: #define BOOST_AUTO_BUFFER_CONSTRUCTOR_SCOPE_GUARD() \ boost::multi_index::detail::make_obj_guard_if<boost::has_nothrow_copy<T>::value>( *this, \ &auto_buffer::deallocate, \ buffer_, members_.capacity_ ) #define BOOST_AUTO_BUFFER_LOCAL_SCOPE_GUARD( BUFFER, CAPACITY ) \ boost::multi_index::detail::make_obj_guard_if<boost::has_nothrow_copy<T>::value>( *this, \ &auto_buffer::deallocate, \ (BUFFER), (CAPACITY) ) Let me know if it is good enough to be committed. regards -Thorsten Index: scope_guard.hpp =================================================================== --- scope_guard.hpp (revision 61481) +++ scope_guard.hpp (working copy) @@ -88,12 +88,48 @@ F fun_; }; +struct null_guard : public scope_guard_impl_base +{ + template< class T1 > + null_guard( const T1& ) + { } + + template< class T1, class T2 > + null_guard( const T1&, const T2& ) + { } + + template< class T1, class T2, class T3 > + null_guard( const T1&, const T2&, const T3& ) + { } + + template< class T1, class T2, class T3, class T4 > + null_guard( const T1&, const T2&, const T3&, const T4& ) + { } + + template< class T1, class T2, class T3, class T4, class T5 > + null_guard( const T1&, const T2&, const T3&, const T4&, const T5& ) + { } +}; + +template< bool is_null, class T > +struct null_guard_return +{ + typedef typename boost::mpl::if_c<is_null,null_guard,T>::type type; +}; + template<typename F> inline scope_guard_impl0<F> make_guard(F fun) { return scope_guard_impl0<F>(fun); } +template<bool is_null, typename F> +inline typename null_guard_return<is_null,scope_guard_impl0<F> >::type +make_guard_if(F fun) +{ + return typename null_guard_return<is_null,scope_guard_impl0<F> >::type(fun); +} + template<typename F,typename P1> class scope_guard_impl1:public scope_guard_impl_base { @@ -113,6 +149,13 @@ return scope_guard_impl1<F,P1>(fun,p1); } +template<bool is_null, typename F,typename P1> +inline typename null_guard_return<is_null,scope_guard_impl1<F,P1> >::type +make_guard_if(F fun,P1 p1) +{ + return typename null_guard_return<is_null,scope_guard_impl1<F,P1> >::type(fun,p1); +} + template<typename F,typename P1,typename P2> class scope_guard_impl2:public scope_guard_impl_base { @@ -133,6 +176,13 @@ return scope_guard_impl2<F,P1,P2>(fun,p1,p2); } +template<bool is_null, typename F,typename P1,typename P2> +inline typename null_guard_return<is_null,scope_guard_impl2<F,P1,P2> >::type +make_guard_if(F fun,P1 p1,P2 p2) +{ + return typename null_guard_return<is_null,scope_guard_impl2<F,P1,P2> >::type(fun,p1,p2); +} + template<typename F,typename P1,typename P2,typename P3> class scope_guard_impl3:public scope_guard_impl_base { @@ -154,6 +204,13 @@ return scope_guard_impl3<F,P1,P2,P3>(fun,p1,p2,p3); } +template<bool is_null,typename F,typename P1,typename P2,typename P3> +inline typename null_guard_return<is_null,scope_guard_impl3<F,P1,P2,P3> >::type +make_guard_if(F fun,P1 p1,P2 p2,P3 p3) +{ + return typename null_guard_return<is_null,scope_guard_impl3<F,P1,P2,P3> >::type(fun,p1,p2,p3); +} + template<typename F,typename P1,typename P2,typename P3,typename P4> class scope_guard_impl4:public scope_guard_impl_base { @@ -178,6 +235,14 @@ return scope_guard_impl4<F,P1,P2,P3,P4>(fun,p1,p2,p3,p4); } +template<bool is_null, typename F,typename P1,typename P2,typename P3,typename P4> +inline typename null_guard_return<is_null,scope_guard_impl4<F,P1,P2,P3,P4> >::type +make_guard_if( + F fun,P1 p1,P2 p2,P3 p3,P4 p4) +{ + return typename null_guard_return<is_null,scope_guard_impl4<F,P1,P2,P3,P4> >::type(fun,p1,p2,p3,p4); +} + template<class Obj,typename MemFun> class obj_scope_guard_impl0:public scope_guard_impl_base { @@ -197,6 +262,13 @@ return obj_scope_guard_impl0<Obj,MemFun>(obj,mem_fun); } +template<bool is_null, class Obj,typename MemFun> +inline typename null_guard_return<is_null,obj_scope_guard_impl0<Obj,MemFun> >::type +make_obj_guard_if(Obj& obj,MemFun mem_fun) +{ + return typename null_guard_return<is_null,obj_scope_guard_impl0<Obj,MemFun> >::type(obj,mem_fun); +} + template<class Obj,typename MemFun,typename P1> class obj_scope_guard_impl1:public scope_guard_impl_base { @@ -219,6 +291,13 @@ return obj_scope_guard_impl1<Obj,MemFun,P1>(obj,mem_fun,p1); } +template<bool is_null, class Obj,typename MemFun,typename P1> +inline typename null_guard_return<is_null,obj_scope_guard_impl1<Obj,MemFun,P1> >::type +make_obj_guard_if( Obj& obj,MemFun mem_fun,P1 p1) +{ + return typename null_guard_return<is_null,obj_scope_guard_impl1<Obj,MemFun,P1> >::type(obj,mem_fun,p1); +} + template<class Obj,typename MemFun,typename P1,typename P2> class obj_scope_guard_impl2:public scope_guard_impl_base { @@ -243,6 +322,13 @@ return obj_scope_guard_impl2<Obj,MemFun,P1,P2>(obj,mem_fun,p1,p2); } +template<bool is_null, class Obj,typename MemFun,typename P1,typename P2> +inline typename null_guard_return<is_null,obj_scope_guard_impl2<Obj,MemFun,P1,P2> >::type +make_obj_guard_if(Obj& obj,MemFun mem_fun,P1 p1,P2 p2) +{ + return typename null_guard_return<is_null,obj_scope_guard_impl2<Obj,MemFun,P1,P2> >::type(obj,mem_fun,p1,p2); +} + template<class Obj,typename MemFun,typename P1,typename P2,typename P3> class obj_scope_guard_impl3:public scope_guard_impl_base { @@ -268,6 +354,13 @@ return obj_scope_guard_impl3<Obj,MemFun,P1,P2,P3>(obj,mem_fun,p1,p2,p3); } +template<bool is_null, class Obj,typename MemFun,typename P1,typename P2,typename P3> +inline typename null_guard_return<is_null,obj_scope_guard_impl3<Obj,MemFun,P1,P2,P3> >::type +make_obj_guard_if(Obj& obj,MemFun mem_fun,P1 p1,P2 p2,P3 p3) +{ + return typename null_guard_return<is_null,obj_scope_guard_impl3<Obj,MemFun,P1,P2,P3> >::type(obj,mem_fun,p1,p2,p3); +} + } /* namespace multi_index::detail */ } /* namespace multi_index */
Thorsten Ottosen <thorsten.ottosen <at> dezide.com> writes:
It looks perfect to me except for two minutiae: * For aesthetic reasons, I'd define null_guard and null_guard_return at line 77, between the definitions of scope_guard and scope_guard_impl0, not after the definition of scope_guard_impl0. * I'd add a reference to make_guard_if to the initial explanatory comment, after line 44. Please go ahead and commit (when the SVN is up again, seemingly there're problems with it right now). Joaquín M López Muñoz Telefónica, Investigación y Desarrollo
On 5/29/2010 6:58 AM, Thorsten Ottosen wrote:
At the risk of intrusively butting in, I couldn't help but notice that Joaquin's specification of make_obj_guard_if was described as (from 2010/05/29 3:41 AM PDT): "I'd prefer to leave make_obj_guard like it is and add an additional make_obj_guard_if<...> (note the _if) for your use case, which creates a null_guard if the condition is not true." However, it appears Thorsten's implementation creates a null_guard if the condition *is* true (the boolean is named "is_null", after all). I'd consider Joaquin's semantics to be less confusing. Also, since I'm here, perhaps you should have a make_obj_guard_if_c taking the boolean template parameter, while make_obj_guard_if takes a Boost.MPL boolean constant class. This is the convention used by Boost.EnableIf and Boost.MPL (among other Boost libraries), right? - Jeff
Jeffrey Lee Hellrung, Jr. skrev:
Oh, I overlooked that. I simple did what felt most natural from a usage perspective, to avoid negating the constant. AFAICT, this the most common usage scenario. Thus when I see make_guard_if<boost::has_nothrow_copy<T>::value>( ... ) I don't read the _if alone (as part of what is being returned), but I read the whole make_guard_if<> as a tool that generates guards as needed depending on has_nothrow_copy<>. Joaquin, what is your take on this?
The if_c<> code is usually pure meta code which is not the case here. -Thorsten
On 5/29/2010 2:31 PM, Thorsten Ottosen wrote:
Certainly, if one sees do_something_if< cond >( ... ) you can sympathize with one reading this as "if cond, do something" ;) Consider those who may not be intimately familiar with make_guard_if but stumble upon such a construct in reviewing code. I think I, for one, would be confused. But the choice of semantics is ultimately up to you and Joaquin.
There is precedent in Boost.Fusion, e.g., at_c and advance_c. - Jeff
Jeffrey Lee Hellrung, Jr. skrev:
On 5/29/2010 2:31 PM, Thorsten Ottosen wrote:
Jeffrey Lee Hellrung, Jr. skrev:
make_guard_if<boost::has_nothrow_copy<T>::value>( ... )
In case that is confusing, I would prefer to spell the functions make_xxx_guard_unless<> or make_xxx_guard_if_not<> -Thorsten
Thorsten Ottosen <thorsten.ottosen <at> dezide.com> writes:
I concur with Jeff's poins of view in both respects: * make_guard_if<cond> should construct the guard if cond is true (I overlooked the fact that your implementation does the opposite, sorry). I think this is the obvious semantics for anyone reading the construct: make a guard if so and so. * It'd be nice if we have both the following: template<typename IsNull, ...> inline typename null_guard_return<..>::type make_guard_if(...) template<bool is_null, ...> inline typename null_guard_return<..>::type make_guard_if_c(...) just like enable_if and enable_if_c, which is probably the closest existing practive we've got in Boost. As for make_guard_unless (and make_guard_unless_c) I guess it's up to you to provide it also, at the risk of being a little overabundant at providing variations on the same basic theme. Joaquín M López Muñoz Telefónica, Investigación y Desarrollo
On 5/30/2010 2:39 AM, Joaquin M Lopez Munoz wrote:
Of course, you mean IsNonNull / is_non_null (or equivalent semantics) for the template parameters...
There *is* disable_if / disable_if_c, and even lazy_* versions, so there is precedent at very explicitly providing such variations. - Jeff
participants (6)
-
Jeffrey Lee Hellrung, Jr. -
Joaquin M Lopez Munoz -
joaquin＠tid.es
-
Stewart, Robert -
Thorsten Ottosen -
Thorsten Ottosen