Enable use of BOOST_FOREACH over const-ref of noncopyable ptr_containers

[Kazutoshi Satoda <k_satoda@…>]

I have encountered an error about allocating abstract type, with the following code:

#include "boost/ptr_container/ptr_vector.hpp"
#include "boost/foreach.hpp"
struct S { virtual void f() const = 0; };
typedef boost::ptr_vector<S> ptr_vector;
void f(ptr_vector const& v) { BOOST_FOREACH(S const& s, v) s.f(); }

And I found this is a known issue: Boost.Foreach > Extensibility

Then I thought that boost::foreach::is_noncopyable<> should be specialized for ptr_containers of which value_type is abstruct. Then I put the following:

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

... and the error disappeared.

After that, since ptr_container is designed to be an alternative of the standard containers for the case of the value is non-copyable, I thought this is a wide problem worth making a patch.

The patch looks better than the above workaround in some points:

• fixes the problem for all ptr_containers at once
• also supports types marked by boost::noncopyable
• keeps the container copyable if view_clone_allocator is used

The patch also includes some test cases I actually used, while I have no idea how to integrate it into boost test infrastructure.

[Kazutoshi Satoda <k_satoda@…>]

The fix and test cases

[Kazutoshi Satoda <k_satoda@…>]

svn diff for trunk r60445

[Kazutoshi Satoda <k_satoda@…>]

svn diff for trunk r60445

[Kazutoshi Satoda <k_satoda@…>]

svn diff for trunk r60445

[Kazutoshi Satoda <k_satoda@…>]

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]

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> {};
}}

[Kazutoshi Satoda <k_satoda@…>]

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]

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.

[Kazutoshi Satoda <k_satoda@…>]

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

[anonymous]

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.

[Kazutoshi Satoda <k_satoda@…>]

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.

[Kazutoshi Satoda <k_satoda@…>]

svn diff for trunk r70851