...

The boost::swap utility forwards its task to a helper function, ::boost_swap_impl::swap_impl. [...] I think it's preferable to remove the top-level namespace, "boost_swap_impl", and move the helper function into the boost namespace. For instance within a new nested namespace, boost::swap_impl_detail.

Joseph Gauterin wrote:

I'm not sure about that - I tried removing it after we removed the ADL barrier and it caused a stack overflow (MSVC8) on some of the tests.

That looks very, very weird to me... And now I see, I get a stack overfow as well, after moving boost_swap_impl::swap_impl to the boost namespace, within my local copy of trunk/boost/utility/swap.hpp. I tried MSVC9 (SP1 not yet applied). My local copy of trunk/libs/utility/swap/test/primitive.cpp says:

Running 1 test case... unknown location(0): fatal error in "test_main_caller( argc, argv )": stack overflow

I'm clueless now. How could moving boost_swap_impl::swap_impl to the boost namespace possibly cause a stack overflow? Is it a known MSVC issue? Kind regards, Niels

...

How could moving boost_swap_impl::swap_impl to the boost namespace possibly cause a stack overflow?

David Abrahams wrote:

Sounds like an infinite recursion to me.

Yes, it is... The following little test program works fine when using the current trunk version of utility/swap.hpp. But it gets into infinite recursion when swap_impl is moved to the boost namespace: //////////////////////////////////////// #include <boost/swap.hpp> int main() { int i1 = 0; int i2 = 9; boost::swap(i1, i2); return i2; } //////////////////////////////////////// After moving swap_impl into the boost namespace, Visual Studio 2008 shows me the following call stack, before the overflow: boost::swap_impl<int>(int & left=0, int & right=9) boost::swap<int,int>(int & left=0, int & right=9) ... boost::swap_impl<int>(int & left=0, int & right=9) boost::swap<int,int>(int & left=0, int & right=9) boost::swap_impl<int>(int & left=0, int & right=9) boost::swap<int,int>(int & left=0, int & right=9) boost::swap_impl<int>(int & left=0, int & right=9) boost::swap<int,int>(int & left=0, int & right=9) main() __tmainCRTStartup() mainCRTStartup() So apparently, within boost::swap_impl, boost::swap<int,int> would be preferred over std::swap<int>, even while std::swap<T> is more specialized than boost::swap<T1,T2>. Isn't that weird? Note that the current version of swap_impl has a using-directive: template<class T> void swap_impl(T& left, T& right) { using namespace std; swap(left,right); } If swap_impl would have a using-declaration instead, "using std::swap", the infinite recursion would not occur. (But we've just replaced the using-declaration by a using-directive, to work around some ADL compiler bugs!) I still wonder if this infinite recursion is a compiler bug as well. But anyway, it's clear to me now that there's a good reason to keep swap_impl out of the boost namespace. :-) Kind regards, Niels

Isaac Dupree wrote:

(please include some explanatory comments!:-)

Yes, I think you're right, <boost/utility/swap.hpp> deserves some more lines of comment.

// various workarounds to avoid compiler bugs that make the compiler // find boost::swap before std::swap make us // - put this outside the boost namespace, // - use a using-directive rather than a using-declaration, // - and make boost::swap have two template arguments.

Thanks for your input! Quite nice actually, but not yet entirely correct: the using-directive is there to work around some other compiler bugs! Both MSVC 7.1 and Borland didn't do argument-dependent lookup when ::boost_swap_impl::swap_impl had a using-declaration ("using std::swap"). While now they do, due to Joseph's commit [47967] :-) See also the thread, "[boost] [swap] Workaround for ADL failures of MSVC 7.1 and Borland okay?", http://lists.boost.org/Archives/boost/2008/08/140578.php So I've just edited your comments a little bit: // Note: the implementation of the boost::swap utility contains // various workarounds: // - swap_impl is put outside the boost namespace, to avoid infinite // recursion (stack overflow) when swapping objects of a primitive type. // - swap_impl has a using-directive, rather than a using-declaration, // because some compilers (including MSVC 7.1 and Borland 5.9.3) don't // do argument-dependent lookup when it has a using-declaration instead. // - boost::swap has two template arguments, instead of one, to // avoid ambiguity when swapping objects of a Boost type that does // not have its own boost::swap overload. Okay? I wouldn't mind adding it to utility/swap.hpp, and committing it to the trunk.

Although I couldn't say how strong the use-case is; probably it's rarely if ever a good idea to specify *that* template argument explicitly.

Okay. I'm not sure if my swap<animal_base_class>(cat,dog) use-case is very strong either. So as long as nobody really feels the need to have direct access to ::boost_swap_impl::swap_impl, I wouldn't mind leaving it outside of the boost namespace.

The function name already confuses me a little though: why should a function named boost::swap do ADL when the nearly equivalent name std::swap doesn't?

Because of its name, the boost::swap utility is overloaded by swap functions from other Boost libraries. And of course, when boost::swap is called for a type that has its own boost::swap overload, this overload will be called directly. Isn't that nice? :-) By the way, boost::exception is also quite different from std::exception. Even more so!

The name swap_using_adl, besides that not everyone knows the acronym "adl", makes it less obvious whether it can find the generic std::swap implementation (through 'using' not ADL).

Indeed, you're right. I was actually considering to spell it out: swap_using_argument_dependent_lookup(T&,T&). But do you have a better suggestion? What about just renaming the function to "call_swap(T&,T&)"? Because that's what it does: it just calls the most appropriate swap function. Anyway, its name is not that important, as long as the helper function remains an "implementation detail".

But maybe the main function has to be named boost::swap to convince clueless people to use it instead of std::swap...

Hope you're convinced already... Kind regards, Niels

David Abrahams wrote:

...

But do you have a better suggestion? What about just renaming the function to "call_swap(T&,T&)"? Because that's what it does: it just calls the most appropriate swap function.

Or, you might say that it "swaps" its arguments. Hey, maybe we should call it swap(T&,T&)!

either "call_swap" or "swap" seems fine with me; I think call_swap specifies the purpose nicely amongst all the ways to call "swap". But likely it's good enough to leave the rationale for having a "boost::swap" for readers to look-up in the Boost documentation, too. On the possibility of calling specific overloads for swapping boost types directly, iff we name it "swap": is there any compiler where that actually makes a difference? -Isaac

David Abrahams wrote:

Or, you might say that it "swaps" its arguments. Hey, maybe we should call it swap(T&,T&)!

...

Anyway, its name is not that important,

I think the name is important.

I hope it's clear that at that point we were discussing the name of boost_swap_impl::swap_impl. Its name would become far more important if it would become part of the public interface. At the moment, it's only intended to be an "implementation detail". On the other hand, the following lines were about the name of the boost::swap utility:

...

Because of its name, the boost::swap utility is overloaded by swap functions from other Boost libraries. And of course, when boost::swap is called for a type that has its own boost::swap overload, this overload will be called directly. Isn't that nice? :-)

Isaac Dupree wrote:

On the possibility of calling specific overloads for swapping boost types directly, iff we name it "swap": is there any compiler where that actually makes a difference?

It clearly saves two function calls. (Boost's swap utility calling swap_impl, and swap_impl calling the most appropriate swap.) Which /might/ be relevant when running in debug mode. At least, it wouldn't harm. :-) Anyway, I'm certainly in favor of having Boost's swap utility named boost::swap. Kind regards, Niels

Joseph Gauterin wrote:

the important thing is that a call to boost::swap swaps its arguments, not the exact mechanism by which it swaps the arguments.

I would like to add some extra checks to the existing test programs in trunk/libs/utility/swap/test, checking if boost::swap indeed swaps its arguments: swap_test_class object1 = initial_value1; swap_test_class object2 = initial_value2; boost::swap(object1,object2); BOOST_CHECK(object1 == initial_value2); BOOST_CHECK(object2 == initial_value1); Therefore, your swap_test_class would need to become EqualComparible, and it would need to have some data. Hope you think it's okay if I apply the attached patch... :-)

Also, there's a clear precedent with the standard library - the algorithm are all named after what they do, not how they do it (swap, sort, remove etc).

The Standard actually describes std::swap in terms of exchanging values: template<class T> void swap(T& a, T& b); ... Effects: Exchanges values stored in two locations. So we could consider renaming "swap_impl" to "exchange_values". (And have boost::swap calling exchange_values.) What do you think?

...

// - swap_impl is put outside the boost namespace, to avoid infinite // recursion (stack overflow) when swapping objects of a primitive type.

Does the infinite recursion only happen using primitive types - and if so do you know why?

I just tested moving ::boost_swap_impl::swap_impl to ::boost::swap_impl on my local copy, and running utility/swap/test for MSVC 7.1, MSVC 9.0, and Borland 5.82. On MSVC (both 7.1 and 9.0), moving ::boost_swap_impl::swap_impl to ::boost::swap_impl introduces test failures

If it does, then it might be possible to move the boost_swap_impl namespace inside namespace boost using some sort of SFINAE technique to explicitly call std::swap for primitive types.