Re: [boost] [bind][tr1] patch for more compliant placeholders

From: Daniel Walker (daniel.j.walker_at_[hidden])
Date: 2007-04-27 10:41:51

On 4/26/07, Peter Dimov <pdimov_at_[hidden]> wrote:
> Daniel Walker wrote:
> > Hello,
> >
> > TR1 section 3.6.4 says that placeholders should be default and copy
> > constructible. However, on gcc and borland boost::bind placeholders
> > are not copy constructible. I tried the following C++0x test.
> >
> > #include <boost/tr1/functional.hpp>
> > #include <boost/concept_check.hpp>
> >
> > int main()
> > {
> >    boost::function_requires<
> >        boost::DefaultConstructible<
> >            typeof(std::tr1::placeholders::_1)
> >        >
> >    >();
> >    boost::function_requires<
> >        boost::CopyConstructible<
> >            typeof(std::tr1::placeholders::_1)
> >        >
> >    >();
> > }
> >
> > This works on gcc 4.1 with GNU's TR1 implementation, but when using
> > Boost.TR1 the second concept check fails to compile.
>
> [...]
>
> > The attached patch does this by introducing a macro
> > BOOST_BIND_ENABLE_INLINE_PLACEHOLDERS. When it is defined
> > placeholders are not objects with internal linkage, and their types do not
> > meet
> > the TR1 concept requirements; i.e. they're inline functions. When the
> > macro is not defined placeholders are objects with internal linkage,
> > and their types are both default and copy constructible. The macro is
> > undefined by default. If a user needs inline placeholders for
> > precompiled headers or whatever reason, they can make the trade off
> > and define the macro. The patch also includes user documentation.
>
> I'm not at all sure that breaking the code of all boost::bind users who
> happen to use precompiled headers with Borland and g++ is acceptable.

I agree that breaking code should be avoided if at all possible. And
good news! I just ran the test suite with g++ 4.1 using precompiled
bind.hpp and mem_fn.hpp and everything passed with object
placeholders. What version of g++ had broken PCH and needed the inline
placeholders? Inline placeholders could be enabled for that version
using BOOST_TESTED_AT. Perhaps, it's better not to mention PCH in the
documentation and just make my change based on the compiler's version
number.

As for Borland, based on your comments in cvs (thanks!), I think this
change is innocuous. From Borland's perspective, it's basically just
reverting placeholder.hpp back to revision 1.3. I can try to test it
with borland 5.5 later.

Incidental, if PCH support on g++ is a big priority, some changes
should probably be made to the test files in order to run the test
suite with precompiled headers. Basically, #include <boost/bind.hpp>
more or less needs to be the first line of each file. It may also be a
good idea to add PCH set up to the Jamfile since g++ makes you jump
through hoops to precompile headers with .hpp extensions.

> On first reading, I was inclined to finish the sentence with "... merely in
> order to make an irrelevant concept check pass". On second reading, you
> probably have in mind the fact that Lambda takes its arguments by const
> reference (boost::bind takes them by value and the deduced type of _1 *is*
> CopyConstructible in this case.)

Actually, what led me down this road is that it's not possible to
overload operators for built-in types like functions or function
pointers. Lambda overloads operators for placeholders, for example
_1++. So, what I'm trying to do with lambda assumes more than the
concept requirements in TR1, but presumable most TR1 implementations
will use classes or enums for placeholders. Implementations that don't
will not be able to use TR1 placeholders in lambda expressions.
They'll still have lambda placeholders until the Phoenix merger, I
assume, and maybe after that. But independent of my lambda hacking, I
didn't see any reason that gcc users (at least for recent versions of
gcc) should be forced into using placeholder types that don't meet the
TR1 concept requirements. Actually, maybe the TR1 requirements should
be broadened in the final ISO draft to explicitly mandate that
placeholders be of class or enum types.

>
> I agree that this is a problem if we want placeholder interoperability under
> g++ and I'm not sure what is the best way to address the issue.

Well, sticking to a standard set of type requirements is a good start
towards insuring interoperability, which is why concept checks are
useful in testing. I think lambda can work with TR1 placeholders on
g++ using GNU's TR1 implementation (and on other compilers using
Boost.TR1). It would be a shame if it didn't also work on g++ (and
borland) using Boost.TR1 since your original bind implementation is in
many ways the mother of them all and one of the primary reasons bind
is in TR1 in the first place!

> I'm fairly certain that we ought to leave Borland users alone, though.

Like I said, I don't think this change will impact borland users,
other than making placeholders more interoperable. I'll try to test it
later though.

Daniel