[Bug Sprint] [variant] #2839 ambiguous swap call
I've just added a test program to ticket #2839: it fails on vc9. The ticket, I think, explains the problem pretty well, but I would like an expert to look at the code. I believe that the program is ill-formed, and it only works on gcc because the BOOST_WORKAROUND disables the code in question. The issue is that variant/detail/move.hpp:114 introduces a templated swap() overload in the same scope it will be called from, to allow move emulation. But, if the variant bounds a type defined in std::, like std::vector, then std::swap is another templated swap() and will be found by ADL. This makes the call ambiguous. My recommendation is that the move-emulation code be removed -- or enable_if'd, if that's possible -- until the generalised Boost.Move implementation is ready to be adopted. If we believe the code as it stands is correct, then I think the test case should be added to trunk and release, and additional BOOST_WORKAROUNDs added for all the compilers where the test fails. -- “Papa Hegel he say that all we learn from history is that we learn noth- hing from history. I know people who can't even learn from what happened this morning. Hegel must have been taking the long view.” -- John Brunner, ‘Stand on Zanzibar’
We solved a similar problem in Boost.Swap by providing a templated swap with two template arguments [making it less specialized than std::swap]. I've had a quick look at variant/detail/move.hpp and I think it can be fixed in the same way.
On Tue, Jun 02, 2009 at 11:10:46AM +0100, Joseph Gauterin wrote:
We solved a similar problem in Boost.Swap by providing a templated swap with two template arguments [making it less specialized than std::swap]. I've had a quick look at variant/detail/move.hpp and I think it can be fixed in the same way.
That's great. I will cook a patch to that effect and attach it to the ticket. -- "I think you look like the Mona Lisa. You always seem to be at a window admiring the landscape that is actually behind you." Herve Le Tellier http://surreal.istic.org/ Habit is a good slave but a poor master.
On Tue, Jun 02, 2009 at 01:45:36PM +0100, Daniel Hulme wrote:
On Tue, Jun 02, 2009 at 11:10:46AM +0100, Joseph Gauterin wrote:
We solved a similar problem in Boost.Swap by providing a templated swap with two template arguments [making it less specialized than std::swap]. I've had a quick look at variant/detail/move.hpp and I think it can be fixed in the same way.
That's great. I will cook a patch to that effect and attach it to the ticket.
Actually, on second thoughts, I don't think I want to do that. I'm pretty sure the intent of the code is to replace the generic swap-by-copy-assignment std::swap template, but *not* to replace a specifically-written swap. Making it *less* specialized than std::swap is effectively the same as removing detail::move_swap::swap(), I think. Is there a hack to exclude the std::swap template from consideration for this call? -- Unless obstructed through environmental noise or great distance, holler- ing can be used to request line discipline from the link partner in State Idle. The use of cellphones is also an option, whereas throwing objects or using guns is not recommended. RFC 4824
I'm pretty sure the intent of the code is to replace the generic swap-by-copy-assignment std::swap template
The intent is to replace std::swap with a call a specialized swap (via ADL) or to call boost::detail::move_swap::swap otherwise. std::swap will never be called from that function, unless std::swap is picked up by ADL - in which case it probably *is* specialized for the type being swapped. By making boost::detail::move_swap::swap less specialized than std::swap, you can stop the ambiguity that happens only in this case. For example, in your test case where you are swapping a std::vector you actually want std::swap to be called, as it is the specialized version (the standard defines the effect of template <class T, class Alloc> std::swap(std::vector<T,Alloc>& x, std::vector<T,Alloc>& y); to be x.swap(y)). For clarity; It's boost::detail::move_swap::swap that needs changing to have two tempalte parameters. Joe.
On Wed, Jun 03, 2009 at 07:38:51PM +0100, Joseph Gauterin wrote:
I'm pretty sure the intent of the code is to replace the generic swap-by-copy-assignment std::swap template
The intent is to replace std::swap with a call a specialized swap (via ADL) or to call boost::detail::move_swap::swap otherwise. std::swap will never be called from that function, unless std::swap is picked up by ADL - in which case it probably *is* specialized for the type being swapped. By making boost::detail::move_swap::swap less specialized than std::swap, you can stop the ambiguity that happens only in this case.
OK, yes, I'll buy that. I'm not convinced that there is nothing in ::std without a std::swap specialization, but I guess I am convinced that for any of those things it makes no difference whether we call std::swap or detail::move_swap::swap. Maybe. I think I will write an additional test case that checks a specialized swap is being called if it exists, just to make me feel safer. I won't be able to get round to this until Monday, though, as I'm out of town, so if anyone wants to do it before then, I won't be offended. :-) Thanks for all your help. -- "Sometimes it's a Boat, and sometimes it's more of an Accident. It all "Depends on what?" depends." "On whether I'm on the top of it or underneath it." -- A. A. Milne, ‘Winnie-the-Pooh’ http://surreal.istic.org/
AMDG Daniel Hulme wrote:
On Wed, Jun 03, 2009 at 07:38:51PM +0100, Joseph Gauterin wrote:
I'm pretty sure the intent of the code is to replace the generic swap-by-copy-assignment std::swap template
The intent is to replace std::swap with a call a specialized swap (via ADL) or to call boost::detail::move_swap::swap otherwise. std::swap will never be called from that function, unless std::swap is picked up by ADL - in which case it probably *is* specialized for the type being swapped. By making boost::detail::move_swap::swap less specialized than std::swap, you can stop the ambiguity that happens only in this case.
OK, yes, I'll buy that. I'm not convinced that there is nothing in ::std without a std::swap specialization, but I guess I am convinced that for any of those things it makes no difference whether we call std::swap or detail::move_swap::swap. Maybe.
classes in namespace std are not a problem. In the worst case, we could enumerate them. The real problem is that arbitrary user defined types can have namespace std as an associated namespace. In Christ, Steven Watanabe
Joseph Gauterin wrote:
The real problem is that arbitrary user defined types can have namespace std as an associated namespace. How can that happen? Do you mean users specializing std::swap for a user defined type?
template < typename T > class user_template; typedef user_template< std::string > user_type; Here user_type has std as (one of) its associated namespace. This is one example, there are other cases as well... HTH, Gevorg
I'd still go for making boost::detail::move_swap::swap less specialized, as it is the only option I can see that will result in valid code for types where ADL considers std::swap, even if it could result in sub-optimal performance. That is unless anyone can think of a clever way to circumvent the problem.
participants (4)
-
Daniel Hulme -
Gevorg Voskanyan -
Joseph Gauterin -
Steven Watanabe