Eric Niebler wrote:

MB wrote:

...

Hi,

Under VC++7.1, I was surprised that a class derived from 'boost::iterator_range' was almost copied by BOOST_FOREACH. ('boost::noncopyable' works!)

I think such a range that is noncopyable but derived from iterator_range is valid, isn't it ...? I think it could be dangerous if users can't find cheap_copy-customization.

Hrm, right. I feel some explanation is in order. The support for const rvalue detection is not without runtime cost, and it's all to avoid needless copying of expensive range-like objects such as STL containers. But when the range is something small like a boost::iterator_range, jumping through hoops of fire to avoid copying one is just silly. Hence the cheap_copy hack: BOOST_FOREACH has an optimized code path for types it knows (or believes to be) cheap to copy.

You have run afoul of the cheap_copy optimization. Rightly or wrongly, it assumes types derived from boost::iterator_range are cheap to copy, but that's not the case for you. There are two possible fixes, and they both involve documenting the cheap_copy customization point (and probably fixing it to use ADL like boost::begin()):

1) By default, only apply the optimization to boost:iterator_range and its ilk, NOT to types derived from them.

2) Leave the code as it is, and force people in your situation to use the cheap_copy customization point to disable the optimization.

I currently lean toward (1).

I will vote for (2) only because almost all my ranges are cheap-copyable and derived from iterator_range :-) But I don't know for sure. I learnt from books to vote for (1). Regards, MB http://p-stade.sourceforge.net/

Eric Niebler <eric@boost-consulting.com> writes:

MB wrote:

...

Eric Niebler wrote:

...

1) By default, only apply the optimization to boost:iterator_range and its ilk, NOT to types derived from them.

2) Leave the code as it is, and force people in your situation to use the cheap_copy customization point to disable the optimization.

I currently lean toward (1).

I will vote for (2) only because almost all my ranges are cheap-copyable and derived from iterator_range :-)

OK, I've gone with (2) for now. There's still time to change it before 1.34 in case anyone hollers. I have turned cheap_copy into an ADL customization point. It is now called boost_foreach_has_cheap_copy, and you define an overload as:

inline boost::mpl::true_ * boost_foreach_has_cheap_copy(your_type_here *) { return 0; }

Put the overload in an associated namespace of the type. Or for maximum portability, you could put the overload at global scope.

Really? If invoked from within a template, global scope overloads won't be found unless an argument is in the global namespace or the overloads happen to appear before the point of definition of the template. If you want a way to make this portable, consider adding a dummy parameter that comes from namespace Boost and asking people to overload there (the Ramey trick). -- Dave Abrahams Boost Consulting www.boost-consulting.com

"Eric Niebler" <eric@boost-consulting.com> writes:

David Abrahams wrote:

...

Eric Niebler <eric@boost-consulting.com> writes:

...

Put the overload in an associated namespace of the type. Or for maximum portability, you could put the overload at global scope.

Really? If invoked from within a template, global scope overloads won't be found unless an argument is in the global namespace or the overloads happen to appear before the point of definition of the template.

That was the idea, yes.

...

If you want a way to make this portable, consider adding a dummy parameter that comes from namespace Boost and asking people to overload there (the Ramey trick).

I'm not sure how this helps on compilers that don't do ADL. This is a macro. BOOST_FOREACH(foo, bar) will expand to something that makes a non-qualified call to boost_foreach_has_cheap_copy *in the context of the BOOST_FOREACH invocation*. This could be any context, in any namespace.

Right. Including from within a template, which could foil lookup in the global namespace.

If I want this to work on compilers that don't do ADL, requiring people to make their overloads visible at global scope seems necessary, but I've likely overlooked something.

namespace boost { template <class T> struct foreach_has_cheap_copy { char (& test(...)) [2]; char test(mpl::true_*); typedef mpl::bool_< sizeof( test( boost_foreach_has_cheap_copy( (T*)0, boost::whatever()) )) > type; }; } Now all compilers will look in namespace boost, whether or not they support ADL. -- Dave Abrahams Boost Consulting www.boost-consulting.com

Eric Niebler wrote:

Templates don't "foil lookup in the global namespace." They merely require that the overload has been seen already. I have no problem requiring users of broken compilers to put the overload where BOOST_FOREACH can find it. Users of non-broken compilers can put the overload in an associated namespace and be happy.

That's what I do. Non-ADL compilers usually don't have two-phase lookup, so it balances out.

Eric Niebler wrote:

Peter Dimov wrote:

...

Eric Niebler wrote:

...

Templates don't "foil lookup in the global namespace." They merely require that the overload has been seen already. I have no problem requiring users of broken compilers to put the overload where BOOST_FOREACH can find it. Users of non-broken compilers can put the overload in an associated namespace and be happy.

That's what I do. Non-ADL compilers usually don't have two-phase lookup, so it balances out.

Actually, I just found a compromise: make the global namespace an associated namespace. Now, the recommended procedure for customizing BOOST_FOREACH is to define a function:

inline boost::mpl::true_ * boost_foreach_has_cheap_copy(your_type*, boost::foreach::tag) { return 0; }

This can go in an associated namespace of your_type /or/ for maximum portability in the global namespace. (boost::foreach::tag is a typedef for an enum_at_global_scope_with_a_really_long_ugly_name.) This allows for a very simple implementation of BOOST_FOREACH that even VC6 seems happy with.

I tried boost::foreach::tag-version under VC++7.1. 'to_ptr' makes: warning C4100: 't' : unreferenced formal parameter Anyway I think it is ok for "me". But I think about the case that my ranges which are customized as 'cheap_copy' are derived from anyone else. Won't the same problem as 'iterator_range<>*' come? Now that 'has_cheap_copy' becomes one of the requirements of Range-concepts, doesn't it? I don't care, but I think foreach is so primitive that professionals should consider that. In general, do we have the safe way of telling whether or not a type is cheap to copy without metafunctions? Regards, MB http://p-stade.sourceforge.net/

Eric Niebler wrote:

MB wrote:

...

Eric Niebler wrote:

...

Now, the recommended procedure for customizing BOOST_FOREACH is to define a function:

inline boost::mpl::true_ * boost_foreach_has_cheap_copy(your_type*, boost::foreach::tag) { return 0; }

This can go in an associated namespace of your_type /or/ for maximum portability in the global namespace. (boost::foreach::tag is a typedef for an enum_at_global_scope_with_a_really_long_ugly_name.) This allows for a very simple implementation of BOOST_FOREACH that even VC6 seems happy with.

I tried boost::foreach::tag-version under VC++7.1.

'to_ptr' makes: warning C4100: 't' : unreferenced formal parameter

I'll fix that.

...

Anyway I think it is ok for "me".

But I think about the case that my ranges which are customized as 'cheap_copy' are derived from anyone else. Won't the same problem as 'iterator_range<>*' come?

Sorry, I don't understand the question. Are you saying you think it's wrong to assume that types derived from iterator_range are cheap to copy?

Errata: But I think about the case that my ranges which are customized as 'cheap_copy' becomes base classes of anyone else. replaces: >But I think about the case that my ranges which are customized as 'cheap_copy' >are derived from anyone else. English is as difficult as C++ :-)

...

Now that 'has_cheap_copy' becomes one of the requirements of Range-concepts, doesn't it?

No. Boost.Range defines the range concept, and FOREACH works correctly with any type that satisfies that concept. has_cheap_copy is nothing more than an optimization hint to the BOOST_FOREACH macro. It is not required and it doesn't change the meaning of anything.

But the hint expands to the derived classes and the derived classes "must" customize 'has_cheap_copy'.

...

In general, do we have the safe way of telling whether or not a type is cheap to copy without metafunctions?

No.

Is it portability-issue the reason why you avoid metafunctions? Regards, MB http://p-stade.sourceforge.net/

Eric Niebler wrote:

...

But the hint expands to the derived classes and the derived classes "must" customize 'has_cheap_copy'.

I agree that's not good. I've reimplemented the has_cheap_copy customization point again. See below.

...

Is it portability-issue the reason why you avoid metafunctions?

You mean, why isn't there a has_cheap_copy<T> trait? I had originally wanted the cheap_copy optimization to automatically apply to derived types, which is the reason for the particular form of the customization point. I no longer think that's a good idea, so I can now add a has_cheap_copy<> trait, however ...

... on compilers without partial template specialization, there isn't a good way of saying that for all types T, boost::iterator_range<T> is cheap to copy. So the free function template is still the way to go on those compilers.

Here is the new-and-improved(-and-please-god-let-this-be-the-last) way to optimize FOREACH for cheap-to-copy types:

- For a user-defined range type Foo that is cheap to copy, you should specialize has_cheap_copy<> in the boost::foreach namespace, as:

namespace boost { namespace foreach { template<> struct has_cheap_copy<Foo> : mpl::true_ {}; }}

- For maximum portability, you could achieve the same effect by overloaded boost_foreach_has_cheap_copy() at global scope like this:

inline boost::mpl::true_ * boost_foreach_has_cheap_copy(Foo *&, boost::foreach::tag) { return 0; }

(The slightly strange Foo *& type prevents this overload from being selected for types derived from Foo.)

By default FOREACH defines overloads to recognize std::pair<T,T>, iterator_range<T> and sub_range<T> as cheap-to-copy, but NOT types derived from them.

I hope this is satisfactory.

I think you are right. My misgiving was how common a range which is noncopyable but derived from iterator_range is. My sixth range was such a one. Though I still wonder what range is cheap to copy, ('iterator_range' of 'spirit::file_iterator' that has 'shared_ptr' is cheap to copy?) optimization-issues now don't surprise any legal ranges. Regards, MB http://p-stade.sourceforge.net/