Boost C++ Libraries: Ticket #3996: Enable use of BOOST_FOREACH over const-ref of noncopyable ptr_containers

attachment set

Wed, 10 Mar 2010 20:25:40 GMT

attachment → ptr_container_propagate_noncopyable_for_foreach.patch

The fix and test cases

attachment set

Thu, 11 Mar 2010 15:29:49 GMT

attachment → foreach_separate_headers_for_extensibility.patch

svn diff for trunk r60445

attachment set

Thu, 11 Mar 2010 15:32:41 GMT

attachment → foreach_separate_headers_for_extensibility.2.patch

svn diff for trunk r60445

attachment set

Thu, 11 Mar 2010 15:33:47 GMT

attachment → ptr_container_propagate_noncopyable_for_foreach.2.patch

svn diff for trunk r60445

Thu, 11 Mar 2010 15:39:06 GMT

Sorry but, the first patch broke all tests which doesn't include boost/foreach.hpp.

Now I attached a new patch. The new one depends on another patch submitted as ticket #3998.

... sorry again, I did mistakes attaching a patch. Please remove two wrong patches (foreach_....patch, they are for #3998), and the first patch. I seem not having the permission to do that by myself.

Eric Niebler — Thu, 24 Jun 2010 22:17:58 GMT

Do NOT accept this patch. For every instantiation of reversible_ptr_container, you are instantiating foreach::is_noncopyable to (optionally) inherit from noncopyable. This is the wrong way to do it and will lead to ODR violations when different translation units see different specializations of foreach::is_noncopyable. Instead, define a partial specialization of foreach::is_noncopyable< reversible_ptr_container<...> >:

namespace boost { namespace foreach {
  template<typename T, typename A, typenale CA>
  struct is_noncopyable<ptr_vector<T,A,CA> > : is_noncopyable<T> {};
}}

Fri, 25 Jun 2010 00:20:40 GMT

Replying to eric_niebler:

... and will lead to ODR violations when different translation units see different specializations of foreach::is_noncopyable.

How different specializations of foreach::is_noncopyable<> can be seen for a ValueType which is required to be complete for the instantiation of foreach::is_noncopyable<ValueType> in my patch?

I can imagine a case where the specialization of foreach::is_noncopyable<ValueType> is provided in a separate header from one which defines ValueType. But in such case, the problem is in ValueType; the same problem can be caused without my patch.

Instead, define a partial specialization of foreach::is_noncopyable< reversible_ptr_container<...> >

Providing a specialization of foreach::is_noncopyable<> for a base class isn't propagated to derived classes (ptr_vector<...>, ptr_list<...>, ... in this case). With that approach, duplicated definitions of specialization would be required. I think it's hardly acceptable, especially thinking users can define their own ptr_container using reversible_ptr_container<>.

Eric Niebler — Fri, 25 Jun 2010 01:03:28 GMT

1) A type ThirdParty is defined in ThirdParty.h. The user can't edit this file because it's not hers. It does not specialize is_noncopyable because it knows nothing about BOOST_FOREACH.

2) A user creates her own ThirdPartyForEach.h that #includes ThirdParty.h and specializes is_noncopyable. Where ever she wants to use ThirdParty together with BOOST_FOREACH, she includes ThirdPartyForEach.h

3) In Unit1.cpp, the user instantiates ptr_vector<ThirdParty>. She includes ThirdParty.h instead of ThirdPartyForEach.h because in this translation unit she is not using BOOST_FOREACH.

4) In Unit2.cpp, she also instantiates ptr_vector<ThirdParty>. She *does* include ThirdPartyForEach.h in this translation unit because it uses BOOST_FOREACH.

5) Undefined behavior.

The problem stems from the fact that the user cannot possibly know that she needs to include ThirdPartyForEach.h EVEN IN TRANSLATION UNITS THAT DON'T USE BOOST_FOREACH. This is a recipe for ODR violations.

Contrast this with my suggested solution. is_noncopyable will *only* be instantiated in the translation units that actually use BOOST_FOREACH. In these source files, it's natural to expect people to include the header that correctly implements the foreach hooks.

Sat, 26 Jun 2010 04:07:54 GMT

Replying to eric_niebler: Thank you for the clarification. Now I agree. I'll try to make another patch.

anonymous — Sat, 11 Dec 2010 14:20:57 GMT

Is there any news about this issue? I have encountered it myself today and I think, I will rather not use BOOST_FOREACH than inserting a workaround somewhere in my project.

status changed; resolution set

Thorsten Ottosen — Thu, 31 Mar 2011 20:24:30 GMT

status new → closed
resolution → fixed

status, version, milestone changed; resolution deleted

Sat, 02 Apr 2011 11:06:19 GMT

status closed → reopened
version Boost 1.42.0 → Boost Development Trunk
resolution fixed
milestone Boost 1.43.0 → To Be Determined

Replying to nesotto: I verified that the problem is still not fixed at trunk r70851. I assume you missed the problem because you tested with a compiler that enables BOOST_FOREACH_COMPILE_TIME_CONST_RVALUE_DETECTION.

Now, I made the 3rd patch. This version contains a test which will can show the problem in all compilers, and takes care of ODR violation which Eric Niebler pointed out.

Only one test "const_element_containers" fails on my machine (cygwinx86, g++ 4.3.4):

const_element_containers.cpp:19: error: expected primary-expression before 'template' const_element_containers.cpp:19: error: expected `;' before 'template' ...

But I'm assuming this is not caused by my change because it is syntax errors in the test code.

attachment set

Sat, 02 Apr 2011 11:08:41 GMT

attachment → ptr_container_propagate_noncopyable_for_foreach.3.patch

svn diff for trunk r70851

cc set

Fri, 20 Jul 2012 19:32:45 GMT

cc jonathan.jones@… added