[swap] Workaround for ADL failures of MSVC 7.1 and Borland okay?
The boost::swap utility (svn/boost/trunk/boost/utility/swap.hpp) still may call std::swap when the user has added a custom swap function to the namespace of the user's type, because of some compiler bugs regarding argument-dependent lookup (ADL). This issue will occur when using GCC 3.3.1, Intel C++ 8.1, MSVC 7.1, or Borland, as indicated by regression failures of "specialized_in_global" and/or "specialized_in_other" for those specific compilers: http://www.boost.org/development/tests/trunk/developer/utility-swap_.html Those compilers typically fail to do ADL within boost::swap's internal helper function, boost_swap_impl::swap_impl<T>: template<class T> void swap_impl(T& left, T& right) { using std::swap; swap(left,right); // <-- Some compiles don't do ADL here! } What about removing the the using-declaration, "using std::swap;", and adding an extra function, boost_swap_impl::swap<T>? Which would directly call std::swap, as follows: namespace boost_swap_impl { template<class T> void swap(T& left, T& right) { std::swap(left,right); } } After doing so, MSVC 7.1 (2003) will pass both "specialized_in_global" and "specialized_in_other", while it never did before. Borland will pass "specialized_in_other" for the first time, as I tested locally on BCC 5.8.4. Who knows, maybe it will also improve the regression results of GCC 3.3.1 and Intel C++ 8.1... :-) Moreover, I think that such a workaround could also be useful to other Boost libraries, when they would start to use the boost::swap utility. For example, it could solve some swap related regression failures of boost::optional. What do you think? Would it be okay to commit this change to the trunk? Hereby the patch I would like to apply. Kind regards, -- Niels Dekker http://www.xs4all.nl/~nd/dekkerware Scientific programmer at LKEB, Leiden University Medical Center
What about removing the using-declaration, "using std::swap;", and adding an extra function, boost_swap_impl::swap<T>? Which would directly call std::swap, as follows:
namespace boost_swap_impl { template<class T> void swap(T& left, T& right) { std::swap(left,right); } }
Hmmm... not such a good idea of mine! Doing so would lead to ambiguity when the type T would /not/ have its own swap overload, while it would have the std namespace as an associated namespace. For example, when T would be a pointer-to-std::string or an std::time_base::dateorder enum. I wish this boost_swap_impl::swap could have been a little less "specialized"! (But still, it would need to be more specialized than boost::swap itself!) Anyway, I think we should add unit tests for std types! Kind regards, Niels
AMDG Niels Dekker - mail address until 2008-12-31 wrote:
Hmmm... not such a good idea of mine! Doing so would lead to ambiguity when the type T would /not/ have its own swap overload, while it would have the std namespace as an associated namespace. For example, when T would be a pointer-to-std::string or an std::time_base::dateorder enum.
I wish this boost_swap_impl::swap could have been a little less "specialized"! (But still, it would need to be more specialized than boost::swap itself!)
There's a config macro |BOOST_FUNCTION_SCOPE_USING_DECLARATION_BREAKS_ADL |Does it help to a) using namespace std; or b) put the using in the namespace rather than in the function
Anyway, I think we should add unit tests for std types!
Definitely. In Christ, Steven Watanabe
template<class T> void swap_impl(T& left, T& right) { using std::swap; swap(left,right); // <-- Some compilers don't do ADL here! }
Steven Watanabe wrote:
There's a config macro |BOOST_FUNCTION_SCOPE_USING_DECLARATION_BREAKS_ADL|
Thanks! Interestingly, the macro is not defined for MSVC 7.1. And I'm not sure if it should, because for MSVC 7.1, a function scope using-declaration doesn't /always/ break ADL. But it definitely breaks ADL within the current boost_swap_impl::swap_impl<T> function.
Does it help to a) using namespace std; or b) put the using in the namespace rather than in the function
Yes it does!!! :-) Workaround "a" does the job! I tested locally, for msvc-9.0, msvc-7.1, and borland-5.82. Current SVN version of boost::swap: - msvc-9.0 passes all tests - msvc-7.1 fails on specialized_in_global, specialized_in_other - borland-5.82 fails on specialized_in_global, specialized_in_other, and swap_arrays Workaround a) "using namespace std;", instead of "using std::swap;", within the boost_swap_impl::swap_impl<T> function: - msvc-9.0 passes all tests - msvc-7.1 passes all tests - borland-5.82 fails /only/ on swap_arrays Workaround b) putting "using std::swap;" in the boost_swap_impl namespace rather than in the boost_swap_impl::swap_impl<T> function: - msvc-9.0 passes all tests - msvc-7.1 passes all tests - borland-5.82 passes specialized_in_other, but it still fails on specialized_in_global and swap_arrays. Do you think it would be okay to just apply workaround "a", as attached hereby? I mean, would it harm for other toolsets to have "using namespace std;" within the boost_swap_impl::swap_impl function? I'd rather not use BOOST_WORKAROUND (unless really necessary!), because it would be harder to maintain, and I'd rather not have the semantics of boost::swap differ from one compiler to another.
Anyway, I think we should add unit tests for std types! Definitely.
So I just added 7 tests, testing boost::swap on various types that have std as associated namespace: std_bitset.cpp tests swapping std::bitset<T>. std_dateorder.cpp tests swapping the enum std::time_base::dateorder, std_string.cpp tests swapping std::string. std_typeinfo_ptr.cpp tests swapping typeinfo pointers. std_vector_of_boost.cpp tests swapping a vector of boost types. std_vector_of_global.cpp tests swapping a vector of types from the global namespace. std_vector_of_other.cpp tests swapping a vector of types from another namespace. I think each of them /might/ independently fail, depending on compiler bugs or (possibly) boost::swap utility bugs. The code is very much based upon the original tests by Joseph. Please have a look: http://svn.boost.org/trac/boost/changeset/47943 Kind regards, Niels
AMDG Niels Dekker - mail address until 2008-12-31 wrote:
Yes it does!!! :-) Workaround "a" does the job! I tested locally, for msvc-9.0, msvc-7.1, and borland-5.82.
<snip>
Do you think it would be okay to just apply workaround "a", as attached hereby? I mean, would it harm for other toolsets to have "using namespace std;" within the boost_swap_impl::swap_impl function?
I think it's ok. A using directive is just as correct as a using declaration, so it shouldn't break any of the more conformant compilers. I've tested it on gcc and como. In Christ, Steven Watanabe
[...] would it harm for other toolsets to have "using namespace std;" within the boost_swap_impl::swap_impl function?
Steven Watanabe wrote:
I think it's ok. A using directive is just as correct as a using declaration, so it shouldn't break any of the more conformant compilers. I've tested it on gcc and como.
Thanks, Steve! I'm glad to hear that a "using namespace std" directive appears to work fine, for more conformant, as well as for less conformant compilers. I'm off now until August 13, so I think I'll just commit the workaround to the trunk when I'm back. (If it's still needed by then, of course!) Kind regards, Niels
participants (2)
-
Niels Dekker - mail address until 2008-12-31 -
Steven Watanabe