[range] concept checks break chained range adaptors
Hi Neil, the fix for #9851 broke some of our code. We use chained range adaptors like std::vector<std::string> separated std::set<Foo> foos; boost::copy(separated | boost::adaptors::transformed(boost::bind(Foo::fromString, _1)) | boost::adaptors::filtered(boost::bind(&Foo::isValid, _1)), std::inserter(foos, foos.end())); which fails with error: constructor for 'boost::transform_iterator<boost::_bi::bind_t<Foo, Foo (*)(const std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > &), boost::_bi::list1<boost::arg<1> > >, std::__1::__wrap_iter<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > *>, boost::use_default, boost::use_default>' must explicitly initialize the member 'm_f' which does not have a default constructor transform_iterator() { } Full testcase and error message attached. This compiles and works with 1.55.0. git bisect shows that this fails to compile from 5f2560f - Ticket 9851 - adaptors should perform concept check assertions. for gcc and for clang starting with e43796c - allow clang to use Range Concepts despite reporting GCC 4.2. msvc-12.0 fails, too. gcc-4.8.2 and clang-3.5 (trunk) for the record. Any chance to get this fixed for 1.56.0 ? Yours, Jürgen -- * Dipl.-Math. Jürgen Hunold ! * voice: ++49 4257 300 ! Fährstraße 1 * fax : ++49 4257 300 ! 31609 Balge/Sebbenhausen * jhunold@gmx.eu ! Germany
Hi! On Sat, Jun 14, 2014 at 2:26 PM, Jürgen Hunold <jhunold@gmx.eu> wrote:
Hi Neil,
the fix for #9851 broke some of our code. We use chained range adaptors like
This fix simply enables BOOST_RANGE_CONCEPT_ASSERT. I had previously disabled this checking on more compilers than I should have. This was motivated to get code up and running on compilers that I didn't have access to. I have since had users have a number of difficulties due that have been difficult to diagnose due to having concept checking disabled. This is particularly the case on later versions of Clang where the C++ support is now excellent. I therefore do not wish to revert the commit for reasons: 1. Any specific problem with particular compilers can be worked around by simply disabling BOOST_RANGE_CONCEPT_ASSERT in boost/range/config.hpp 2. I will find ways of making concept assertions work by changing them in the cases where there are problems 3. I will specifically disable concept assertions for problems for which I fail to find solutions
std::vector<std::string> separated std::set<Foo> foos;
boost::copy(separated | boost::adaptors::transformed(boost::bind(Foo::fromString, _1)) | boost::adaptors::filtered(boost::bind(&Foo::isValid, _1)), std::inserter(foos, foos.end()));
which fails with
error: constructor for 'boost::transform_iterator<boost::_bi::bind_t<Foo, Foo (*)(const std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > &), boost::_bi::list1<boost::arg<1> > >, std::__1::__wrap_iter<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > *>, boost::use_default, boost::use_default>' must explicitly initialize the member 'm_f' which does not have a default constructor transform_iterator() { }
Full testcase and error message attached. This compiles and works with 1.55.0. git bisect shows that this fails to compile from 5f2560f - Ticket 9851 - adaptors should perform concept check assertions. for gcc and for clang starting with e43796c - allow clang to use Range Concepts despite reporting GCC 4.2. msvc-12.0 fails, too. gcc-4.8.2 and clang-3.5 (trunk) for the record.
I really appreciate the thorough report. I shall look at this problem over the next couple of days and update the Trac ticket to report if I believe I can get this fixed for the 1.56 release. My initial reading of this issue is that the problem is with the actual Concepts disallowing valid use cases. Therefore the code before 5f2560f would perhaps have worked on the compilers you were using but would have failed elsewhere. Let's see if we can get it working everywhere. My assumption is that by a minor edit to boost/range/config.hpp you can get back up and running. Please confirm/deny this assumption.
Any chance to get this fixed for 1.56.0 ?
I shall try to within the constraints of the release schedule. It is starting to get a little tight. Hopefully this will only require changes that are backward compatible. I certainly shall provide you with a solution although it might require a small patch if I miss the release window.
Yours,
Jürgen
Sorry for the inconvenience caused by these changes. I hope I can convince you that it is part of a march toward a general improvement so that users can more frequently enjoy the benefits of concept checking. Regards, Neil Groves
Hi Neil, Am Samstag, 14. Juni 2014, 15:27:05 schrieb Neil Groves:
Hi!
On Sat, Jun 14, 2014 at 2:26 PM, Jürgen Hunold <jhunold@gmx.eu> wrote:
Hi Neil,
the fix for #9851 broke some of our code. We use chained range adaptors like
This fix simply enables BOOST_RANGE_CONCEPT_ASSERT. I had previously disabled this checking on more compilers than I should have. This was motivated to get code up and running on compilers that I didn't have access to. I have since had users have a number of difficulties due that have been difficult to diagnose due to having concept checking disabled. This is particularly the case on later versions of Clang where the C++ support is now excellent.
I therefore do not wish to revert the commit for reasons: 1. Any specific problem with particular compilers can be worked around by simply disabling BOOST_RANGE_CONCEPT_ASSERT in boost/range/config.hpp 2. I will find ways of making concept assertions work by changing them in the cases where there are problems 3. I will specifically disable concept assertions for problems for which I fail to find solutions
Well, I don't want you to revert those commits. I'm glad to have found the commit(s) which caused regressions here.
Full testcase and error message attached. This compiles and works with 1.55.0. git bisect shows that this fails to compile from 5f2560f - Ticket 9851 - adaptors should perform concept check assertions. for gcc and for clang starting with e43796c - allow clang to use Range Concepts despite reporting GCC 4.2. msvc-12.0 fails, too. gcc-4.8.2 and clang-3.5 (trunk) for the record.
I really appreciate the thorough report. I shall look at this problem over the next couple of days and update the Trac ticket to report if I believe I can get this fixed for the 1.56 release. My initial reading of this issue is that the problem is with the actual Concepts disallowing valid use cases. Therefore the code before 5f2560f would perhaps have worked on the compilers you were using but would have failed elsewhere. Let's see if we can get it working everywhere.
Well, the test case (and the original code) works with gcc-4.8.x, clang 3.4 and 3.5 and at least msvc-12.0 with Boost 1.55.0
My assumption is that by a minor edit to boost/range/config.hpp you can get back up and running. Please confirm/deny this assumption.
Sorry, you lost me there. I can see no concept check related #defines there. Can you clarify this?
Any chance to get this fixed for 1.56.0 ?
I shall try to within the constraints of the release schedule. It is starting to get a little tight. Hopefully this will only require changes that are backward compatible. I certainly shall provide you with a solution although it might require a small patch if I miss the release window.
I would have spotted this earlier if clang would not disguise as gcc-4.2 (see e43796c) and I'm mostly using clang these days :-(. So no need to hurry, I can apply patches to our internal repository for Boost any time ;-)
Sorry for the inconvenience caused by these changes. I hope I can convince you that it is part of a march toward a general improvement so that users can more frequently enjoy the benefits of concept checking.
No need for excuses, I fully understand your motivation :-) Improvements are always good in the long term. Short term hiccups are to be expected. This is not the first nor will be the last regression our large codebases have caught :-) The main reason I'm regularly using boost's develop branch is to catch those errors before they get released. Yours, Jürgen -- * Dipl.-Math. Jürgen Hunold ! * voice: ++49 4257 300 ! Fährstraße 1 * fax : ++49 4257 300 ! 31609 Balge/Sebbenhausen * jhunold@gmx.eu ! Germany
Hello,
std::vector<std::string> separated std::set<Foo> foos;
boost::copy(separated | boost::adaptors::transformed(boost::bind(Foo::fromString, _1)) | boost::adaptors::filtered(boost::bind(&Foo::isValid, _1)), std::inserter(foos, foos.end()));
boost::copy forwards everything to std::copy with iterators that hold function objects by value. Iterator concept requires iterators to be default constructible, but if they hold function objects that have binded data they cannot be made such. FWIW one of solutions to this problem is wrapping function object f with boost::regular(f), available here: https://github.com/faithandbrave/OvenToBoost AFAIR it's supposed to be integrated into boost some day, and also there was some discussion in this list about it. -- ------------ Sergey Mitsyn.
Hi Sergey, Am Sonntag, 15. Juni 2014, 13:20:58 schrieb Sergey Mitsyn:
Hello, boost::copy forwards everything to std::copy with iterators that hold function objects by value. Iterator concept requires iterators to be default constructible, but if they hold function objects that have binded data they cannot be made such.
Well, the code itself is working great in 1.55.0 and before the aforementionend commits introducing concept checks. So Boost.Range already does the right things. It seems that only the new checks need some tweaking.
FWIW one of solutions to this problem is wrapping function object f with boost::regular(f), available here:
Thanks for the pointer, anyway. Yours, Jürgen -- * Dipl.-Math. Jürgen Hunold ! * voice: ++49 4257 300 ! Fährstraße 1 * fax : ++49 4257 300 ! 31609 Balge/Sebbenhausen * jhunold@gmx.eu ! Germany
participants (3)
-
Jürgen Hunold -
Neil Groves -
Sergey Mitsyn